From 3e82dc1b7b5e0f076dd44cebaec0796eaede0693 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Wed, 16 Jul 2025 12:06:52 +0530 Subject: [PATCH 01/13] [ty] Implement mock language server for testing --- Cargo.lock | 1 + crates/ty_server/Cargo.toml | 1 + crates/ty_server/src/lib.rs | 22 +- crates/ty_server/src/logging.rs | 90 +-- crates/ty_server/src/server.rs | 58 +- crates/ty_server/src/session.rs | 49 +- crates/ty_server/src/session/options.rs | 18 +- ...r__tests__initialization_capabilities.snap | 69 ++ ..._tests__initialization_with_workspace.snap | 69 ++ crates/ty_server/src/system.rs | 48 +- crates/ty_server/src/test.rs | 627 ++++++++++++++++++ 11 files changed, 962 insertions(+), 90 deletions(-) create mode 100644 crates/ty_server/src/snapshots/ty_server__server__tests__initialization_capabilities.snap create mode 100644 crates/ty_server/src/snapshots/ty_server__server__tests__initialization_with_workspace.snap create mode 100644 crates/ty_server/src/test.rs diff --git a/Cargo.lock b/Cargo.lock index 3ee5faec40280c..46062d9a9ef130 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4290,6 +4290,7 @@ dependencies = [ "anyhow", "bitflags 2.9.1", "crossbeam", + "insta", "jod-thread", "libc", "lsp-server", diff --git a/crates/ty_server/Cargo.toml b/crates/ty_server/Cargo.toml index 0a74ebe9f02c61..3b52524470d6af 100644 --- a/crates/ty_server/Cargo.toml +++ b/crates/ty_server/Cargo.toml @@ -38,6 +38,7 @@ tracing = { workspace = true } tracing-subscriber = { workspace = true, features = ["chrono"] } [dev-dependencies] +insta = { workspace = true, features = ["json"] } [target.'cfg(target_vendor = "apple")'.dependencies] libc = { workspace = true } diff --git a/crates/ty_server/src/lib.rs b/crates/ty_server/src/lib.rs index df7c05788395ee..4e4d03dd340ab4 100644 --- a/crates/ty_server/src/lib.rs +++ b/crates/ty_server/src/lib.rs @@ -1,7 +1,8 @@ -use std::num::NonZeroUsize; +use std::{num::NonZeroUsize, sync::Arc}; use anyhow::Context; use lsp_server::Connection; +use ruff_db::system::{OsSystem, SystemPathBuf}; use crate::server::Server; pub use document::{NotebookDocument, PositionEncoding, TextDocument}; @@ -13,6 +14,9 @@ mod server; mod session; mod system; +#[cfg(test)] +pub mod test; + pub(crate) const SERVER_NAME: &str = "ty"; pub(crate) const DIAGNOSTIC_NAME: &str = "ty"; @@ -30,7 +34,21 @@ pub fn run_server() -> anyhow::Result<()> { let (connection, io_threads) = Connection::stdio(); - let server_result = Server::new(worker_threads, connection) + let cwd = { + let cwd = std::env::current_dir().context("Failed to get the current working directory")?; + SystemPathBuf::from_path_buf(cwd).map_err(|path| { + anyhow::anyhow!( + "The current working directory `{}` contains non-Unicode characters. \ + ty only supports Unicode paths.", + path.display() + ) + })? + }; + + // This is to complement the `LSPSystem` if the document is not available in the index. + let fallback_system = Arc::new(OsSystem::new(cwd)); + + let server_result = Server::new(worker_threads, connection, fallback_system) .context("Failed to start server")? .run(); diff --git a/crates/ty_server/src/logging.rs b/crates/ty_server/src/logging.rs index ce92a803ab19ff..917b55858edff6 100644 --- a/crates/ty_server/src/logging.rs +++ b/crates/ty_server/src/logging.rs @@ -4,7 +4,7 @@ //! are written to `stderr` by default, which should appear in the logs for most LSP clients. A //! `logFile` path can also be specified in the settings, and output will be directed there //! instead. -use std::sync::Arc; +use std::sync::{Arc, Once}; use ruff_db::system::{SystemPath, SystemPathBuf}; use serde::Deserialize; @@ -14,51 +14,55 @@ use tracing_subscriber::fmt::time::ChronoLocal; use tracing_subscriber::fmt::writer::BoxMakeWriter; use tracing_subscriber::layer::SubscriberExt; +static INIT_LOGGING: Once = Once::new(); + pub(crate) fn init_logging(log_level: LogLevel, log_file: Option<&SystemPath>) { - let log_file = log_file - .map(|path| { - // this expands `logFile` so that tildes and environment variables - // are replaced with their values, if possible. - if let Some(expanded) = shellexpand::full(&path.to_string()) - .ok() - .map(|path| SystemPathBuf::from(&*path)) - { - expanded - } else { - path.to_path_buf() - } - }) - .and_then(|path| { - std::fs::OpenOptions::new() - .create(true) - .append(true) - .open(path.as_std_path()) - .map_err(|err| { - #[expect(clippy::print_stderr)] - { - eprintln!("Failed to open file at {path} for logging: {err}"); - } - }) - .ok() - }); + INIT_LOGGING.call_once(|| { + let log_file = log_file + .map(|path| { + // this expands `logFile` so that tildes and environment variables + // are replaced with their values, if possible. + if let Some(expanded) = shellexpand::full(&path.to_string()) + .ok() + .map(|path| SystemPathBuf::from(&*path)) + { + expanded + } else { + path.to_path_buf() + } + }) + .and_then(|path| { + std::fs::OpenOptions::new() + .create(true) + .append(true) + .open(path.as_std_path()) + .map_err(|err| { + #[expect(clippy::print_stderr)] + { + eprintln!("Failed to open file at {path} for logging: {err}"); + } + }) + .ok() + }); - let logger = match log_file { - Some(file) => BoxMakeWriter::new(Arc::new(file)), - None => BoxMakeWriter::new(std::io::stderr), - }; - let is_trace_level = log_level == LogLevel::Trace; - let subscriber = tracing_subscriber::Registry::default().with( - tracing_subscriber::fmt::layer() - .with_timer(ChronoLocal::new("%Y-%m-%d %H:%M:%S.%f".to_string())) - .with_thread_names(is_trace_level) - .with_target(is_trace_level) - .with_ansi(false) - .with_writer(logger) - .with_filter(LogLevelFilter { filter: log_level }), - ); + let logger = match log_file { + Some(file) => BoxMakeWriter::new(Arc::new(file)), + None => BoxMakeWriter::new(std::io::stderr), + }; + let is_trace_level = log_level == LogLevel::Trace; + let subscriber = tracing_subscriber::Registry::default().with( + tracing_subscriber::fmt::layer() + .with_timer(ChronoLocal::new("%Y-%m-%d %H:%M:%S.%f".to_string())) + .with_thread_names(is_trace_level) + .with_target(is_trace_level) + .with_ansi(false) + .with_writer(logger) + .with_filter(LogLevelFilter { filter: log_level }), + ); - tracing::subscriber::set_global_default(subscriber) - .expect("should be able to set global default subscriber"); + tracing::subscriber::set_global_default(subscriber) + .expect("should be able to set global default subscriber"); + }); } /// The log level for the server as provided by the client during initialization. diff --git a/crates/ty_server/src/server.rs b/crates/ty_server/src/server.rs index 03b7b8326556f7..357c6b86a62449 100644 --- a/crates/ty_server/src/server.rs +++ b/crates/ty_server/src/server.rs @@ -11,8 +11,9 @@ use lsp_types::{ ServerCapabilities, SignatureHelpOptions, TextDocumentSyncCapability, TextDocumentSyncKind, TextDocumentSyncOptions, TypeDefinitionProviderCapability, Url, WorkDoneProgressOptions, }; +use ruff_db::system::System; use std::num::NonZeroUsize; -use std::panic::PanicHookInfo; +use std::panic::{PanicHookInfo, RefUnwindSafe}; use std::sync::Arc; mod api; @@ -35,7 +36,11 @@ pub(crate) struct Server { } impl Server { - pub(crate) fn new(worker_threads: NonZeroUsize, connection: Connection) -> crate::Result { + pub(crate) fn new( + worker_threads: NonZeroUsize, + connection: Connection, + fallback_system: Arc, + ) -> crate::Result { let (id, init_value) = connection.initialize_start()?; let init_params: InitializeParams = serde_json::from_value(init_value)?; @@ -102,10 +107,14 @@ impl Server { .collect() }) .or_else(|| { - let current_dir = std::env::current_dir().ok()?; + let current_dir = fallback_system + .current_directory() + .as_std_path() + .to_path_buf(); tracing::warn!( "No workspace(s) were provided during initialization. \ - Using the current working directory as a default workspace: {}", + Using the current working directory from the fallback system as a \ + default workspace: {}", current_dir.display() ); let uri = Url::from_file_path(current_dir).ok()?; @@ -143,6 +152,7 @@ impl Server { position_encoding, global_options, workspaces, + fallback_system, )?, client_capabilities, }) @@ -288,3 +298,43 @@ impl Drop for ServerPanicHookHandler { } } } + +#[cfg(test)] +mod tests { + use ruff_db::system::{InMemorySystem, SystemPathBuf}; + + use crate::session::ClientOptions; + use crate::test::TestServerBuilder; + + #[test] + fn initialization_sequence() { + let system = InMemorySystem::default(); + let test_server = TestServerBuilder::new() + .with_memory_system(system) + .build() + .unwrap() + .wait_until_workspaces_are_initialized() + .unwrap(); + + let initialization_result = test_server.initialization_result().unwrap(); + + insta::assert_json_snapshot!("initialization_capabilities", initialization_result); + } + + #[test] + fn initialization_with_workspace() { + let workspace_root = SystemPathBuf::from("/foo"); + let system = InMemorySystem::new(workspace_root.clone()); + let test_server = TestServerBuilder::new() + .with_memory_system(system) + .with_workspace(&workspace_root, ClientOptions::default()) + .build() + .unwrap() + .wait_until_workspaces_are_initialized() + .unwrap(); + + let initialization_result = test_server.initialization_result().unwrap(); + + insta::assert_json_snapshot!("initialization_with_workspace", initialization_result); + } +} diff --git a/crates/ty_server/src/session.rs b/crates/ty_server/src/session.rs index 2137efe483bd0c..6072390f821455 100644 --- a/crates/ty_server/src/session.rs +++ b/crates/ty_server/src/session.rs @@ -2,11 +2,14 @@ use std::collections::{BTreeMap, VecDeque}; use std::ops::{Deref, DerefMut}; +use std::panic::RefUnwindSafe; use std::sync::Arc; use anyhow::{Context, anyhow}; use index::DocumentQueryError; use lsp_server::Message; +use lsp_types::notification::{Exit, Notification}; +use lsp_types::request::{Request, Shutdown}; use lsp_types::{ClientCapabilities, TextDocumentContentChangeEvent, Url}; use options::GlobalOptions; use ruff_db::Db; @@ -37,6 +40,9 @@ mod settings; /// The global state for the LSP pub(crate) struct Session { + /// A fallback system to use with the [`LSPSystem`]. + fallback_system: Arc, + /// Used to retrieve information about open documents and settings. /// /// This will be [`None`] when a mutable reference is held to the index via [`index_mut`] @@ -99,6 +105,7 @@ impl Session { position_encoding: PositionEncoding, global_options: GlobalOptions, workspace_folders: Vec<(Url, ClientOptions)>, + fallback_system: Arc, ) -> crate::Result { let index = Arc::new(Index::new(global_options.into_settings())); @@ -108,6 +115,7 @@ impl Session { } Ok(Self { + fallback_system, position_encoding, workspaces, deferred_messages: VecDeque::new(), @@ -155,6 +163,9 @@ impl Session { } else { match &message { Message::Request(request) => { + if request.method == Shutdown::METHOD { + return Some(message); + } tracing::debug!( "Deferring `{}` request until all workspaces are initialized", request.method @@ -165,6 +176,9 @@ impl Session { return Some(message); } Message::Notification(notification) => { + if notification.method == Exit::METHOD { + return Some(message); + } tracing::debug!( "Deferring `{}` notification until all workspaces are initialized", notification.method @@ -218,9 +232,12 @@ impl Session { /// If the path is a virtual path, it will return the first project database in the session. pub(crate) fn project_state(&self, path: &AnySystemPath) -> &ProjectState { match path { - AnySystemPath::System(system_path) => self - .project_state_for_path(system_path) - .unwrap_or_else(|| self.default_project.get(self.index.as_ref())), + AnySystemPath::System(system_path) => { + self.project_state_for_path(system_path).unwrap_or_else(|| { + self.default_project + .get(self.index.as_ref(), &self.fallback_system) + }) + } AnySystemPath::SystemVirtual(_virtual_path) => { // TODO: Currently, ty only supports single workspace but we need to figure out // which project should this virtual path belong to when there are multiple @@ -247,7 +264,10 @@ impl Session { .range_mut(..=system_path.to_path_buf()) .next_back() .map(|(_, project)| project) - .unwrap_or_else(|| self.default_project.get_mut(self.index.as_ref())), + .unwrap_or_else(|| { + self.default_project + .get_mut(self.index.as_ref(), &self.fallback_system) + }), AnySystemPath::SystemVirtual(_virtual_path) => { // TODO: Currently, ty only supports single workspace but we need to figure out // which project should this virtual path belong to when there are multiple @@ -330,7 +350,10 @@ impl Session { // For now, create one project database per workspace. // In the future, index the workspace directories to find all projects // and create a project database for each. - let system = LSPSystem::new(self.index.as_ref().unwrap().clone()); + let system = LSPSystem::new( + self.index.as_ref().unwrap().clone(), + self.fallback_system.clone(), + ); let project = ProjectMetadata::discover(&root, &system) .context("Failed to discover project configuration") @@ -748,12 +771,16 @@ impl DefaultProject { DefaultProject(std::sync::OnceLock::new()) } - pub(crate) fn get(&self, index: Option<&Arc>) -> &ProjectState { + pub(crate) fn get( + &self, + index: Option<&Arc>, + fallback_system: &Arc, + ) -> &ProjectState { self.0.get_or_init(|| { tracing::info!("Initializing the default project"); let index = index.unwrap(); - let system = LSPSystem::new(index.clone()); + let system = LSPSystem::new(index.clone(), fallback_system.clone()); let metadata = ProjectMetadata::from_options( Options::default(), system.current_directory().to_path_buf(), @@ -771,8 +798,12 @@ impl DefaultProject { }) } - pub(crate) fn get_mut(&mut self, index: Option<&Arc>) -> &mut ProjectState { - let _ = self.get(index); + pub(crate) fn get_mut( + &mut self, + index: Option<&Arc>, + fallback_system: &Arc, + ) -> &mut ProjectState { + let _ = self.get(index, fallback_system); // SAFETY: The `OnceLock` is guaranteed to be initialized at this point because // we called `get` above, which initializes it if it wasn't already. diff --git a/crates/ty_server/src/session/options.rs b/crates/ty_server/src/session/options.rs index fdd651effaa27f..934b87afa1c88d 100644 --- a/crates/ty_server/src/session/options.rs +++ b/crates/ty_server/src/session/options.rs @@ -49,7 +49,7 @@ struct WorkspaceOptions { /// This is a direct representation of the settings schema sent by the client. #[derive(Clone, Debug, Deserialize, Default)] -#[cfg_attr(test, derive(PartialEq, Eq))] +#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))] #[serde(rename_all = "camelCase")] pub(crate) struct ClientOptions { /// Settings under the `python.*` namespace in VS Code that are useful for the ty language @@ -63,7 +63,7 @@ pub(crate) struct ClientOptions { /// Diagnostic mode for the language server. #[derive(Clone, Copy, Debug, Default, Deserialize)] -#[cfg_attr(test, derive(PartialEq, Eq))] +#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))] #[serde(rename_all = "camelCase")] pub(crate) enum DiagnosticMode { /// Check only currently open files. @@ -147,21 +147,21 @@ impl ClientOptions { // all settings and not just the ones in "python.*". #[derive(Clone, Debug, Deserialize, Default)] -#[cfg_attr(test, derive(PartialEq, Eq))] +#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))] #[serde(rename_all = "camelCase")] struct Python { ty: Option, } #[derive(Clone, Debug, Deserialize, Default)] -#[cfg_attr(test, derive(PartialEq, Eq))] +#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))] #[serde(rename_all = "camelCase")] struct PythonExtension { active_environment: Option, } #[derive(Clone, Debug, Deserialize)] -#[cfg_attr(test, derive(PartialEq, Eq))] +#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))] #[serde(rename_all = "camelCase")] pub(crate) struct ActiveEnvironment { pub(crate) executable: PythonExecutable, @@ -170,7 +170,7 @@ pub(crate) struct ActiveEnvironment { } #[derive(Clone, Debug, Deserialize)] -#[cfg_attr(test, derive(PartialEq, Eq))] +#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))] #[serde(rename_all = "camelCase")] pub(crate) struct EnvironmentVersion { pub(crate) major: i64, @@ -182,7 +182,7 @@ pub(crate) struct EnvironmentVersion { } #[derive(Clone, Debug, Deserialize)] -#[cfg_attr(test, derive(PartialEq, Eq))] +#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))] #[serde(rename_all = "camelCase")] pub(crate) struct PythonEnvironment { pub(crate) folder_uri: Url, @@ -194,7 +194,7 @@ pub(crate) struct PythonEnvironment { } #[derive(Clone, Debug, Deserialize)] -#[cfg_attr(test, derive(PartialEq, Eq))] +#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))] #[serde(rename_all = "camelCase")] pub(crate) struct PythonExecutable { #[allow(dead_code)] @@ -203,7 +203,7 @@ pub(crate) struct PythonExecutable { } #[derive(Clone, Debug, Deserialize, Default)] -#[cfg_attr(test, derive(PartialEq, Eq))] +#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))] #[serde(rename_all = "camelCase")] struct Ty { disable_language_services: Option, diff --git a/crates/ty_server/src/snapshots/ty_server__server__tests__initialization_capabilities.snap b/crates/ty_server/src/snapshots/ty_server__server__tests__initialization_capabilities.snap new file mode 100644 index 00000000000000..b7f89f5e2abe35 --- /dev/null +++ b/crates/ty_server/src/snapshots/ty_server__server__tests__initialization_capabilities.snap @@ -0,0 +1,69 @@ +--- +source: crates/ty_server/src/server.rs +expression: initialization_result +--- +{ + "capabilities": { + "positionEncoding": "utf-16", + "textDocumentSync": { + "openClose": true, + "change": 2 + }, + "hoverProvider": true, + "completionProvider": { + "triggerCharacters": [ + "." + ] + }, + "signatureHelpProvider": { + "triggerCharacters": [ + "(", + "," + ], + "retriggerCharacters": [ + ")" + ] + }, + "definitionProvider": true, + "typeDefinitionProvider": true, + "declarationProvider": true, + "semanticTokensProvider": { + "legend": { + "tokenTypes": [ + "namespace", + "class", + "parameter", + "selfParameter", + "clsParameter", + "variable", + "property", + "function", + "method", + "keyword", + "string", + "number", + "decorator", + "builtinConstant", + "typeParameter" + ], + "tokenModifiers": [ + "definition", + "readonly", + "async" + ] + }, + "range": true, + "full": true + }, + "inlayHintProvider": {}, + "diagnosticProvider": { + "identifier": "ty", + "interFileDependencies": true, + "workspaceDiagnostics": false + } + }, + "serverInfo": { + "name": "ty", + "version": "Unknown" + } +} diff --git a/crates/ty_server/src/snapshots/ty_server__server__tests__initialization_with_workspace.snap b/crates/ty_server/src/snapshots/ty_server__server__tests__initialization_with_workspace.snap new file mode 100644 index 00000000000000..b7f89f5e2abe35 --- /dev/null +++ b/crates/ty_server/src/snapshots/ty_server__server__tests__initialization_with_workspace.snap @@ -0,0 +1,69 @@ +--- +source: crates/ty_server/src/server.rs +expression: initialization_result +--- +{ + "capabilities": { + "positionEncoding": "utf-16", + "textDocumentSync": { + "openClose": true, + "change": 2 + }, + "hoverProvider": true, + "completionProvider": { + "triggerCharacters": [ + "." + ] + }, + "signatureHelpProvider": { + "triggerCharacters": [ + "(", + "," + ], + "retriggerCharacters": [ + ")" + ] + }, + "definitionProvider": true, + "typeDefinitionProvider": true, + "declarationProvider": true, + "semanticTokensProvider": { + "legend": { + "tokenTypes": [ + "namespace", + "class", + "parameter", + "selfParameter", + "clsParameter", + "variable", + "property", + "function", + "method", + "keyword", + "string", + "number", + "decorator", + "builtinConstant", + "typeParameter" + ], + "tokenModifiers": [ + "definition", + "readonly", + "async" + ] + }, + "range": true, + "full": true + }, + "inlayHintProvider": {}, + "diagnosticProvider": { + "identifier": "ty", + "interFileDependencies": true, + "workspaceDiagnostics": false + } + }, + "serverInfo": { + "name": "ty", + "version": "Unknown" + } +} diff --git a/crates/ty_server/src/system.rs b/crates/ty_server/src/system.rs index 17e0c67039e280..c8ef6369ae7127 100644 --- a/crates/ty_server/src/system.rs +++ b/crates/ty_server/src/system.rs @@ -1,6 +1,7 @@ use std::any::Any; use std::fmt; use std::fmt::Display; +use std::panic::RefUnwindSafe; use std::sync::Arc; use lsp_types::Url; @@ -8,8 +9,8 @@ use ruff_db::file_revision::FileRevision; use ruff_db::files::{File, FilePath}; use ruff_db::system::walk_directory::WalkDirectoryBuilder; use ruff_db::system::{ - CaseSensitivity, DirectoryEntry, FileType, GlobError, Metadata, OsSystem, PatternError, Result, - System, SystemPath, SystemPathBuf, SystemVirtualPath, SystemVirtualPathBuf, WritableSystem, + CaseSensitivity, DirectoryEntry, FileType, GlobError, Metadata, PatternError, Result, System, + SystemPath, SystemPathBuf, SystemVirtualPath, SystemVirtualPathBuf, WritableSystem, }; use ruff_notebook::{Notebook, NotebookError}; use ty_python_semantic::Db; @@ -118,18 +119,18 @@ pub(crate) struct LSPSystem { /// [`index_mut`]: crate::Session::index_mut index: Option>, - /// A system implementation that uses the local file system. - os_system: OsSystem, + /// A fallback system implementation used when documents are not found in the index. + fallback_system: Arc, } impl LSPSystem { - pub(crate) fn new(index: Arc) -> Self { - let cwd = std::env::current_dir().unwrap(); - let os_system = OsSystem::new(SystemPathBuf::from_path_buf(cwd).unwrap()); - + pub(crate) fn new( + index: Arc, + fallback_system: Arc, + ) -> Self { Self { index: Some(index), - os_system, + fallback_system, } } @@ -183,16 +184,17 @@ impl System for LSPSystem { FileType::File, )) } else { - self.os_system.path_metadata(path) + self.fallback_system.path_metadata(path) } } fn canonicalize_path(&self, path: &SystemPath) -> Result { - self.os_system.canonicalize_path(path) + self.fallback_system.canonicalize_path(path) } fn path_exists_case_sensitive(&self, path: &SystemPath, prefix: &SystemPath) -> bool { - self.os_system.path_exists_case_sensitive(path, prefix) + self.fallback_system + .path_exists_case_sensitive(path, prefix) } fn read_to_string(&self, path: &SystemPath) -> Result { @@ -200,7 +202,7 @@ impl System for LSPSystem { match document { Some(DocumentQuery::Text { document, .. }) => Ok(document.contents().to_string()), - _ => self.os_system.read_to_string(path), + _ => self.fallback_system.read_to_string(path), } } @@ -212,7 +214,7 @@ impl System for LSPSystem { Notebook::from_source_code(document.contents()) } Some(DocumentQuery::Notebook { notebook, .. }) => Ok(notebook.make_ruff_notebook()), - None => self.os_system.read_to_notebook(path), + None => self.fallback_system.read_to_notebook(path), } } @@ -243,26 +245,26 @@ impl System for LSPSystem { } fn current_directory(&self) -> &SystemPath { - self.os_system.current_directory() + self.fallback_system.current_directory() } fn user_config_directory(&self) -> Option { - self.os_system.user_config_directory() + self.fallback_system.user_config_directory() } fn cache_dir(&self) -> Option { - self.os_system.cache_dir() + self.fallback_system.cache_dir() } fn read_directory<'a>( &'a self, path: &SystemPath, ) -> Result> + 'a>> { - self.os_system.read_directory(path) + self.fallback_system.read_directory(path) } fn walk_directory(&self, path: &SystemPath) -> WalkDirectoryBuilder { - self.os_system.walk_directory(path) + self.fallback_system.walk_directory(path) } fn glob( @@ -272,11 +274,11 @@ impl System for LSPSystem { Box> + '_>, PatternError, > { - self.os_system.glob(pattern) + self.fallback_system.glob(pattern) } fn as_writable(&self) -> Option<&dyn WritableSystem> { - self.os_system.as_writable() + self.fallback_system.as_writable() } fn as_any(&self) -> &dyn Any { @@ -288,11 +290,11 @@ impl System for LSPSystem { } fn case_sensitivity(&self) -> CaseSensitivity { - self.os_system.case_sensitivity() + self.fallback_system.case_sensitivity() } fn env_var(&self, name: &str) -> std::result::Result { - self.os_system.env_var(name) + self.fallback_system.env_var(name) } } diff --git a/crates/ty_server/src/test.rs b/crates/ty_server/src/test.rs new file mode 100644 index 00000000000000..81420401ac78b1 --- /dev/null +++ b/crates/ty_server/src/test.rs @@ -0,0 +1,627 @@ +//! Testing server for the ty language server. +//! +//! This module provides mock server infrastructure for testing LSP functionality +//! without requiring actual file system operations or network connections. +//! +//! The design is inspired by the Starlark LSP test server but adapted for ty server architecture. + +use std::collections::hash_map::Entry; +use std::collections::{HashMap, VecDeque}; +use std::fmt; +use std::num::NonZeroUsize; +use std::sync::Arc; +use std::thread::JoinHandle; +use std::time::Duration; + +use anyhow::Result; +use lsp_server::{Connection, Message, RequestId, Response, ResponseError}; +use lsp_types::notification::{DidOpenTextDocument, Exit, Initialized, Notification}; +use lsp_types::request::{Initialize, Request, Shutdown, WorkspaceConfiguration}; +use lsp_types::{ + ClientCapabilities, ConfigurationParams, DiagnosticClientCapabilities, + DidChangeWatchedFilesClientCapabilities, FileEvent, InitializeParams, InitializeResult, + InitializedParams, PublishDiagnosticsClientCapabilities, RegistrationParams, + TextDocumentClientCapabilities, TextDocumentContentChangeEvent, Url, + WorkspaceClientCapabilities, WorkspaceFolder, +}; +use ruff_db::system::{InMemorySystem, SystemPath, TestSystem}; +use serde::de::DeserializeOwned; + +use crate::server::Server; +use crate::session::ClientOptions; + +/// Errors that can occur during testing +#[derive(thiserror::Error, Debug)] +pub(crate) enum TestServerError { + /// The response came back, but was an error response, not a successful one. + #[error("Response error: {0:?}")] + ResponseError(ResponseError), + + #[error("Invalid response message for request {0}: {1:?}")] + InvalidResponse(RequestId, Response), + + #[error("Got a duplicate response for request ID {0}: {1:?}")] + DuplicateResponse(RequestId, Response), + + #[error("Test client received an unrecognized request from the server: {0:?}")] + UnrecognizedRequest(lsp_server::Request), +} + +/// A test server for the ty language server that provides helpers for sending requests, +/// correlating responses, and handling notifications. +pub(crate) struct TestServer { + /// The thread that's actually running the server + server_thread: Option>, + + /// Connection to communicate with the server + client_connection: Connection, + + /// Incrementing counter to automatically generate request IDs + request_counter: i32, + + /// Simple incrementing document version counter + version_counter: i32, + + /// A mapping of request IDs to responses received from the server + responses: HashMap, + + /// An ordered queue of all the notifications received from the server + notifications: VecDeque, + + /// An ordered queue of all the requests received from the server + requests: VecDeque, + + /// How long to wait for messages to be received + recv_timeout: Duration, + + /// The response from server initialization + initialize_response: Option, + + /// Workspace configurations for `workspace/configuration` requests + workspace_configurations: HashMap, + + /// Capabilities registered by the server + registered_capabilities: Vec, +} + +impl Drop for TestServer { + fn drop(&mut self) { + // Follow the LSP protocol to shutdown the server gracefully + match self.send_request::(()) { + Ok(shutdown_id) => match self.get_response::<()>(shutdown_id) { + Ok(()) => { + if let Err(err) = self.send_notification::(()) { + panic!("Failed to send exit notification: {err:?}"); + } + } + Err(err) => { + panic!("Failed to get shutdown response: {err:?}"); + } + }, + Err(err) => { + panic!("Failed to send shutdown request: {err:?}"); + } + } + + if let Some(server_thread) = self.server_thread.take() { + if let Err(err) = server_thread.join() { + panic!("Test server thread did not join when dropped: {err:?}"); + } + } + } +} + +impl fmt::Debug for TestServer { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("TestServer") + .field("request_counter", &self.request_counter) + .field("version_counter", &self.version_counter) + .field("responses", &self.responses) + .field("notifications", &self.notifications) + .field("server_requests", &self.requests) + .field("recv_timeout", &self.recv_timeout) + .field("initialize_response", &self.initialize_response) + .field("workspace_configurations", &self.workspace_configurations) + .field("registered_capabilities", &self.registered_capabilities) + .finish_non_exhaustive() + } +} + +impl TestServer { + /// Create a new test server with the given workspace configurations + pub(crate) fn new( + workspace_folders: Vec, + workspace_configurations: HashMap, + memory_system: InMemorySystem, + capabilities: ClientCapabilities, + ) -> Result { + assert_eq!( + workspace_folders.len(), + workspace_configurations.len(), + "Number of workspace folders should match the number of workspace configurations" + ); + + let (server_connection, client_connection) = Connection::memory(); + + // Start the server in a separate thread + let server_thread = std::thread::spawn(move || { + // TODO: This should probably be configurable to test concurrency issues + let worker_threads = NonZeroUsize::new(1).unwrap(); + let test_system = Arc::new(TestSystem::new(memory_system)); + + match Server::new(worker_threads, server_connection, test_system) { + Ok(server) => { + if let Err(err) = server.run() { + panic!("Server stopped with error: {err:?}"); + } + } + Err(err) => { + panic!("Failed to create server: {err:?}"); + } + } + }); + + Self { + server_thread: Some(server_thread), + client_connection, + request_counter: 0, + version_counter: 0, + responses: HashMap::new(), + notifications: VecDeque::new(), + requests: VecDeque::new(), + recv_timeout: Duration::from_secs(1), + initialize_response: None, + workspace_configurations, + registered_capabilities: Vec::new(), + } + .initialize(workspace_folders, capabilities) + } + + /// Perform LSP initialization handshake + fn initialize( + mut self, + workspace_folders: Vec, + capabilities: ClientCapabilities, + ) -> Result { + let init_params = InitializeParams { + capabilities, + workspace_folders: Some(workspace_folders), + // TODO: This should be configurable by the test server builder + initialization_options: Some(serde_json::Value::Object(serde_json::Map::new())), + ..Default::default() + }; + + let init_request_id = self.send_request::(init_params)?; + self.initialize_response = Some(self.get_response::(init_request_id)?); + self.send_notification::(InitializedParams {})?; + + Ok(self) + } + + /// Wait until the server has initialized all workspaces. + /// + /// This will wait until the client receives a `workspace/configuration` request from the + /// server, and handles the request. + /// + /// This should only be called if the server is expected to send this request. + pub(crate) fn wait_until_workspaces_are_initialized(mut self) -> Result { + let (request_id, params) = self.get_request::()?; + self.handle_workspace_configuration_request(request_id, ¶ms)?; + Ok(self) + } + + /// Generate a new request ID + fn next_request_id(&mut self) -> RequestId { + self.request_counter += 1; + RequestId::from(self.request_counter) + } + + /// Generate a new document version + fn next_document_version(&mut self) -> i32 { + self.version_counter += 1; + self.version_counter + } + + /// Send a request to the server and return the request ID. + /// + /// The caller can use this ID to later retrieve the response using [`get_response`]. + /// + /// [`get_response`]: TestServer::get_response + pub(crate) fn send_request(&mut self, params: R::Params) -> Result + where + R: Request, + { + let id = self.next_request_id(); + let request = lsp_server::Request::new(id.clone(), R::METHOD.to_string(), params); + self.client_connection + .sender + .send(Message::Request(request))?; + Ok(id) + } + + /// Send a notification to the server. + pub(crate) fn send_notification(&self, params: N::Params) -> Result<()> + where + N: Notification, + { + let notification = lsp_server::Notification::new(N::METHOD.to_string(), params); + self.client_connection + .sender + .send(Message::Notification(notification))?; + Ok(()) + } + + /// Get a server response for the given request ID. + /// + /// The request should have already been sent using [`send_request`]. + /// + /// [`send_request`]: TestServer::send_request + pub(crate) fn get_response(&mut self, id: RequestId) -> Result { + loop { + self.receive()?; + + if let Some(response) = self.responses.get(&id) { + match response { + Response { + error: None, + result: Some(result), + .. + } => { + return Ok(serde_json::from_value::(result.clone())?); + } + Response { + error: Some(err), + result: None, + .. + } => { + return Err(TestServerError::ResponseError(err.clone()).into()); + } + response => { + return Err(TestServerError::InvalidResponse(id, response.clone()).into()); + } + } + } + } + } + + /// Get a notification of the specified type from the server and return its parameters. + /// + /// The caller should ensure that the server is expected to send this notification type. It + /// will keep polling the server for notifications up to 10 times before giving up. It can + /// return an error if the notification is not received within `recv_timeout` duration. + #[expect(dead_code)] + pub(crate) fn get_notification(&mut self) -> Result { + for _ in 0..10 { + self.receive()?; + let notification = self + .notifications + .iter() + .enumerate() + .find_map(|(index, notification)| { + if N::METHOD == notification.method { + Some(index) + } else { + None + } + }) + .and_then(|index| self.notifications.remove(index)); + if let Some(notification) = notification { + return Ok(serde_json::from_value(notification.params)?); + } + } + Err(anyhow::anyhow!( + "Did not get a notification of type `{}` in 10 retries", + N::METHOD + )) + } + + /// Get a request of the specified type from the server and return the request ID and + /// parameters. + /// + /// The caller should ensure that the server is expected to send this request type. It will + /// keep polling the server for requests up to 10 times before giving up. It can return an + /// error if the request is not received within `recv_timeout` duration. + pub(crate) fn get_request(&mut self) -> Result<(RequestId, R::Params)> { + for _ in 0..10 { + self.receive()?; + let request = self + .requests + .iter() + .enumerate() + .find_map(|(index, request)| { + if R::METHOD == request.method { + Some(index) + } else { + None + } + }) + .and_then(|index| self.requests.remove(index)); + if let Some(request) = request { + let params = serde_json::from_value(request.params)?; + return Ok((request.id, params)); + } + } + Err(anyhow::anyhow!( + "Did not get a request of type `{}` in 10 retries", + R::METHOD + )) + } + + /// Receive a message from the server. + /// + /// It will wait for `recv_timeout` duration for a message to arrive. If no message is received + /// within that time, it will return an error. + /// + /// Once a message is received, it will store it in the appropriate queue: + /// - Requests are stored in `requests` + /// - Responses are stored in `responses` + /// - Notifications are stored in `notifications` + fn receive(&mut self) -> Result<()> { + let message = self + .client_connection + .receiver + .recv_timeout(self.recv_timeout)?; + + match message { + Message::Request(request) => { + self.requests.push_back(request); + } + Message::Response(response) => match self.responses.entry(response.id.clone()) { + Entry::Occupied(existing) => { + return Err(TestServerError::DuplicateResponse( + response.id, + existing.get().clone(), + ) + .into()); + } + Entry::Vacant(entry) => { + entry.insert(response); + } + }, + Message::Notification(notification) => { + self.notifications.push_back(notification); + } + } + + Ok(()) + } + + /// Handle workspace configuration requests from the server. + /// + /// Use the [`get_request`] method to wait for the server to send this request. + /// + /// [`get_request`]: TestServer::get_request + pub(crate) fn handle_workspace_configuration_request( + &mut self, + request_id: RequestId, + params: &ConfigurationParams, + ) -> Result<()> { + let mut results = Vec::new(); + + for item in ¶ms.items { + let Some(scope_uri) = &item.scope_uri else { + unimplemented!("Handling global configuration requests is not implemented yet"); + }; + let config_value = if let Some(options) = self.workspace_configurations.get(scope_uri) { + // Return the configuration for the specific workspace + match item.section.as_deref() { + Some("ty") => serde_json::to_value(options)?, + Some(_) | None => { + // TODO: Handle `python` section once it's implemented in the server + // As per the spec: + // + // > If the client can’t provide a configuration setting for a given scope + // > then null needs to be present in the returned array. + serde_json::Value::Null + } + } + } else { + // TODO: Should we return an error? User should explicitly configure the test + // server to have a configuration for all workspaces even if they are empty. + serde_json::Value::Null + }; + results.push(config_value); + } + + let response = Response::new_ok(request_id, results); + self.client_connection + .sender + .send(Message::Response(response))?; + + Ok(()) + } + + /// Handle requests from the server (like configuration requests) + #[expect(dead_code)] + fn handle_server_request(&mut self, request: lsp_server::Request) -> Result<()> { + match request.method.as_str() { + "workspace/configuration" => { + let params: ConfigurationParams = serde_json::from_value(request.params)?; + self.handle_workspace_configuration_request(request.id, ¶ms)?; + } + "workspace/diagnostic/refresh" => { + // TODO: Send diagnostic requests for all the open files to the server and send the + // workspace diagnostics request if the server supports it. + let response = Response::new_ok(request.id, serde_json::Value::Null); + self.client_connection + .sender + .send(Message::Response(response))?; + } + "client/registerCapability" => { + // TODO: We might have to expand this to handle more complex registrations and also + // handle unregistration. + let params: RegistrationParams = serde_json::from_value(request.params)?; + for registration in params.registrations { + self.registered_capabilities.push(registration.method); + } + // Accept capability registration requests + let response = Response::new_ok(request.id, serde_json::Value::Null); + self.client_connection + .sender + .send(Message::Response(response))?; + } + _ => { + return Err(TestServerError::UnrecognizedRequest(request).into()); + } + } + + Ok(()) + } + + /// Get the initialization result + pub(crate) fn initialization_result(&self) -> Option<&InitializeResult> { + self.initialize_response.as_ref() + } + + /// Send a `textDocument/didOpen` notification + #[expect(dead_code)] + pub(crate) fn open_text_document(&mut self, uri: Url, content: String) -> Result<()> { + let params = lsp_types::DidOpenTextDocumentParams { + text_document: lsp_types::TextDocumentItem { + uri, + language_id: "python".to_string(), + version: self.next_document_version(), + text: content, + }, + }; + self.send_notification::(params) + } + + /// Send a `textDocument/didChange` notification with the given content changes + #[expect(dead_code)] + pub(crate) fn change_text_document( + &mut self, + uri: Url, + changes: Vec, + ) -> Result<()> { + let params = lsp_types::DidChangeTextDocumentParams { + text_document: lsp_types::VersionedTextDocumentIdentifier { + uri, + version: self.next_document_version(), + }, + content_changes: changes, + }; + self.send_notification::(params) + } + + /// Send a `textDocument/didClose` notification + #[expect(dead_code)] + pub(crate) fn close_text_document(&mut self, uri: Url) -> Result<()> { + let params = lsp_types::DidCloseTextDocumentParams { + text_document: lsp_types::TextDocumentIdentifier { uri }, + }; + self.send_notification::(params) + } + + /// Send a `workspace/didChangeWatchedFiles` notification with the given file events + #[expect(dead_code)] + pub(crate) fn did_change_watched_files(&mut self, events: Vec) -> Result<()> { + let params = lsp_types::DidChangeWatchedFilesParams { changes: events }; + self.send_notification::(params) + } +} + +/// Builder for creating test servers with specific configurations +pub(crate) struct TestServerBuilder { + workspace_folders: Vec, + workspace_configurations: HashMap, + memory_system: InMemorySystem, + client_capabilities: ClientCapabilities, +} + +impl TestServerBuilder { + /// Create a new builder + pub(crate) fn new() -> Self { + // Default client capabilities for the test server. These are assumptions made by the real + // server and are common for most clients: + // + // - Supports publishing diagnostics + // - Supports pulling workspace configuration + let client_capabilities = ClientCapabilities { + text_document: Some(TextDocumentClientCapabilities { + publish_diagnostics: Some(PublishDiagnosticsClientCapabilities::default()), + ..Default::default() + }), + workspace: Some(WorkspaceClientCapabilities { + configuration: Some(true), + ..Default::default() + }), + ..Default::default() + }; + + Self { + workspace_folders: Vec::new(), + workspace_configurations: HashMap::new(), + memory_system: InMemorySystem::default(), + client_capabilities, + } + } + + /// Add a workspace configuration + pub(crate) fn with_workspace( + mut self, + workspace_root: &SystemPath, + options: ClientOptions, + ) -> Self { + let workspace_folder = WorkspaceFolder { + uri: Url::from_file_path(workspace_root.as_std_path()) + .expect("workspace root must be a valid URL"), + name: workspace_root.file_name().unwrap_or("test").to_string(), + }; + self.workspace_configurations + .insert(workspace_folder.uri.clone(), options); + self.workspace_folders.push(workspace_folder); + self + } + + /// Set the in-memory system + pub(crate) fn with_memory_system(mut self, memory_system: InMemorySystem) -> Self { + self.memory_system = memory_system; + self + } + + /// Enable or disable pull diagnostics capability + #[expect(dead_code)] + pub(crate) fn with_pull_diagnostics(mut self, enabled: bool) -> Self { + self.client_capabilities + .text_document + .get_or_insert_with(Default::default) + .diagnostic = if enabled { + Some(DiagnosticClientCapabilities::default()) + } else { + None + }; + self + } + + /// Enable or disable file watching capability + #[expect(dead_code)] + pub(crate) fn with_did_change_watched_files(mut self, enabled: bool) -> Self { + self.client_capabilities + .workspace + .get_or_insert_with(Default::default) + .did_change_watched_files = if enabled { + Some(DidChangeWatchedFilesClientCapabilities::default()) + } else { + None + }; + self + } + + /// Set custom client capabilities (overrides any previously set capabilities) + #[expect(dead_code)] + pub(crate) fn with_client_capabilities(mut self, capabilities: ClientCapabilities) -> Self { + self.client_capabilities = capabilities; + self + } + + /// Build the test server + pub(crate) fn build(self) -> Result { + TestServer::new( + self.workspace_folders, + self.workspace_configurations, + self.memory_system, + self.client_capabilities, + ) + } +} From 626c58531fcbc7bddf1cfc0df2a726fa6ce2f020 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Thu, 17 Jul 2025 20:06:27 +0530 Subject: [PATCH 02/13] Test publish and pull diagnostics --- crates/ty_server/src/server.rs | 58 ++++++++++- ...ests__publish_diagnostics_on_did_open.snap | 99 +++++++++++++++++++ ...__tests__pull_diagnostics_on_did_open.snap | 93 +++++++++++++++++ crates/ty_server/src/test.rs | 78 ++++++++++----- 4 files changed, 302 insertions(+), 26 deletions(-) create mode 100644 crates/ty_server/src/snapshots/ty_server__server__tests__publish_diagnostics_on_did_open.snap create mode 100644 crates/ty_server/src/snapshots/ty_server__server__tests__pull_diagnostics_on_did_open.snap diff --git a/crates/ty_server/src/server.rs b/crates/ty_server/src/server.rs index 357c6b86a62449..98f2368aa7dda2 100644 --- a/crates/ty_server/src/server.rs +++ b/crates/ty_server/src/server.rs @@ -301,7 +301,9 @@ impl Drop for ServerPanicHookHandler { #[cfg(test)] mod tests { - use ruff_db::system::{InMemorySystem, SystemPathBuf}; + use anyhow::Result; + use lsp_types::notification::PublishDiagnostics; + use ruff_db::system::{InMemorySystem, MemoryFileSystem, SystemPathBuf}; use crate::session::ClientOptions; use crate::test::TestServerBuilder; @@ -337,4 +339,58 @@ mod tests { insta::assert_json_snapshot!("initialization_with_workspace", initialization_result); } + + #[test] + fn publish_diagnostics_on_did_open() -> Result<()> { + let workspace_root = SystemPathBuf::from("/src"); + + let fs = MemoryFileSystem::with_current_directory(&workspace_root); + let foo = workspace_root.join("foo.py"); + let foo_content = "\ +def foo() -> str: + return 42 +"; + fs.write_file(&foo, foo_content)?; + + let mut server = TestServerBuilder::new() + .with_memory_system(InMemorySystem::from_memory_fs(fs)) + .with_workspace(&workspace_root, ClientOptions::default()) + .with_pull_diagnostics(false) + .build()? + .wait_until_workspaces_are_initialized()?; + + server.open_text_document(&foo, &foo_content)?; + let diagnostics = server.get_notification::()?; + + insta::assert_debug_snapshot!(diagnostics); + + Ok(()) + } + + #[test] + fn pull_diagnostics_on_did_open() -> Result<()> { + let workspace_root = SystemPathBuf::from("/src"); + + let fs = MemoryFileSystem::with_current_directory(&workspace_root); + let foo = workspace_root.join("foo.py"); + let foo_content = "\ +def foo() -> str: + return 42 +"; + fs.write_file(&foo, foo_content)?; + + let mut server = TestServerBuilder::new() + .with_memory_system(InMemorySystem::from_memory_fs(fs)) + .with_workspace(&workspace_root, ClientOptions::default()) + .with_pull_diagnostics(true) + .build()? + .wait_until_workspaces_are_initialized()?; + + server.open_text_document(&foo, &foo_content)?; + let diagnostics = server.document_diagnostic_request(&foo)?; + + insta::assert_debug_snapshot!(diagnostics); + + Ok(()) + } } diff --git a/crates/ty_server/src/snapshots/ty_server__server__tests__publish_diagnostics_on_did_open.snap b/crates/ty_server/src/snapshots/ty_server__server__tests__publish_diagnostics_on_did_open.snap new file mode 100644 index 00000000000000..e936c4403e7e62 --- /dev/null +++ b/crates/ty_server/src/snapshots/ty_server__server__tests__publish_diagnostics_on_did_open.snap @@ -0,0 +1,99 @@ +--- +source: crates/ty_server/src/server.rs +expression: diagnostics +--- +PublishDiagnosticsParams { + uri: Url { + scheme: "file", + cannot_be_a_base: false, + username: "", + password: None, + host: None, + port: None, + path: "/src/foo.py", + query: None, + fragment: None, + }, + diagnostics: [ + Diagnostic { + range: Range { + start: Position { + line: 1, + character: 11, + }, + end: Position { + line: 1, + character: 13, + }, + }, + severity: Some( + Error, + ), + code: Some( + String( + "invalid-return-type", + ), + ), + code_description: Some( + CodeDescription { + href: Url { + scheme: "https", + cannot_be_a_base: false, + username: "", + password: None, + host: Some( + Domain( + "ty.dev", + ), + ), + port: None, + path: "/rules", + query: None, + fragment: Some( + "invalid-return-type", + ), + }, + }, + ), + source: Some( + "ty", + ), + message: "Return type does not match returned value: expected `str`, found `Literal[42]`", + related_information: Some( + [ + DiagnosticRelatedInformation { + location: Location { + uri: Url { + scheme: "file", + cannot_be_a_base: false, + username: "", + password: None, + host: None, + port: None, + path: "/src/foo.py", + query: None, + fragment: None, + }, + range: Range { + start: Position { + line: 0, + character: 13, + }, + end: Position { + line: 0, + character: 16, + }, + }, + }, + message: "Expected `str` because of return type", + }, + ], + ), + tags: None, + data: None, + }, + ], + version: Some( + 1, + ), +} diff --git a/crates/ty_server/src/snapshots/ty_server__server__tests__pull_diagnostics_on_did_open.snap b/crates/ty_server/src/snapshots/ty_server__server__tests__pull_diagnostics_on_did_open.snap new file mode 100644 index 00000000000000..c6fc9120044ff5 --- /dev/null +++ b/crates/ty_server/src/snapshots/ty_server__server__tests__pull_diagnostics_on_did_open.snap @@ -0,0 +1,93 @@ +--- +source: crates/ty_server/src/server.rs +expression: diagnostics +--- +Report( + Full( + RelatedFullDocumentDiagnosticReport { + related_documents: None, + full_document_diagnostic_report: FullDocumentDiagnosticReport { + result_id: None, + items: [ + Diagnostic { + range: Range { + start: Position { + line: 1, + character: 11, + }, + end: Position { + line: 1, + character: 13, + }, + }, + severity: Some( + Error, + ), + code: Some( + String( + "invalid-return-type", + ), + ), + code_description: Some( + CodeDescription { + href: Url { + scheme: "https", + cannot_be_a_base: false, + username: "", + password: None, + host: Some( + Domain( + "ty.dev", + ), + ), + port: None, + path: "/rules", + query: None, + fragment: Some( + "invalid-return-type", + ), + }, + }, + ), + source: Some( + "ty", + ), + message: "Return type does not match returned value: expected `str`, found `Literal[42]`", + related_information: Some( + [ + DiagnosticRelatedInformation { + location: Location { + uri: Url { + scheme: "file", + cannot_be_a_base: false, + username: "", + password: None, + host: None, + port: None, + path: "/src/foo.py", + query: None, + fragment: None, + }, + range: Range { + start: Position { + line: 0, + character: 13, + }, + end: Position { + line: 0, + character: 16, + }, + }, + }, + message: "Expected `str` because of return type", + }, + ], + ), + tags: None, + data: None, + }, + ], + }, + }, + ), +) diff --git a/crates/ty_server/src/test.rs b/crates/ty_server/src/test.rs index 81420401ac78b1..d0883ac3f4cdec 100644 --- a/crates/ty_server/src/test.rs +++ b/crates/ty_server/src/test.rs @@ -15,14 +15,22 @@ use std::time::Duration; use anyhow::Result; use lsp_server::{Connection, Message, RequestId, Response, ResponseError}; -use lsp_types::notification::{DidOpenTextDocument, Exit, Initialized, Notification}; -use lsp_types::request::{Initialize, Request, Shutdown, WorkspaceConfiguration}; +use lsp_types::notification::{ + DidChangeTextDocument, DidChangeWatchedFiles, DidCloseTextDocument, DidOpenTextDocument, Exit, + Initialized, Notification, +}; +use lsp_types::request::{ + DocumentDiagnosticRequest, Initialize, Request, Shutdown, WorkspaceConfiguration, +}; use lsp_types::{ ClientCapabilities, ConfigurationParams, DiagnosticClientCapabilities, - DidChangeWatchedFilesClientCapabilities, FileEvent, InitializeParams, InitializeResult, - InitializedParams, PublishDiagnosticsClientCapabilities, RegistrationParams, - TextDocumentClientCapabilities, TextDocumentContentChangeEvent, Url, - WorkspaceClientCapabilities, WorkspaceFolder, + DidChangeTextDocumentParams, DidChangeWatchedFilesClientCapabilities, + DidChangeWatchedFilesParams, DidCloseTextDocumentParams, DidOpenTextDocumentParams, + DocumentDiagnosticParams, DocumentDiagnosticReportResult, FileEvent, InitializeParams, + InitializeResult, InitializedParams, PartialResultParams, PublishDiagnosticsClientCapabilities, + RegistrationParams, TextDocumentClientCapabilities, TextDocumentContentChangeEvent, + TextDocumentIdentifier, TextDocumentItem, Url, VersionedTextDocumentIdentifier, + WorkDoneProgressParams, WorkspaceClientCapabilities, WorkspaceFolder, }; use ruff_db::system::{InMemorySystem, SystemPath, TestSystem}; use serde::de::DeserializeOwned; @@ -289,7 +297,6 @@ impl TestServer { /// The caller should ensure that the server is expected to send this notification type. It /// will keep polling the server for notifications up to 10 times before giving up. It can /// return an error if the notification is not received within `recv_timeout` duration. - #[expect(dead_code)] pub(crate) fn get_notification(&mut self) -> Result { for _ in 0..10 { self.receive()?; @@ -474,14 +481,17 @@ impl TestServer { } /// Send a `textDocument/didOpen` notification - #[expect(dead_code)] - pub(crate) fn open_text_document(&mut self, uri: Url, content: String) -> Result<()> { - let params = lsp_types::DidOpenTextDocumentParams { - text_document: lsp_types::TextDocumentItem { - uri, + pub(crate) fn open_text_document( + &mut self, + path: impl AsRef, + content: &impl ToString, + ) -> Result<()> { + let params = DidOpenTextDocumentParams { + text_document: TextDocumentItem { + uri: Url::from_file_path(path.as_ref()).expect("Path must be a valid URL"), language_id: "python".to_string(), version: self.next_document_version(), - text: content, + text: content.to_string(), }, }; self.send_notification::(params) @@ -491,33 +501,52 @@ impl TestServer { #[expect(dead_code)] pub(crate) fn change_text_document( &mut self, - uri: Url, + path: impl AsRef, changes: Vec, ) -> Result<()> { - let params = lsp_types::DidChangeTextDocumentParams { - text_document: lsp_types::VersionedTextDocumentIdentifier { - uri, + let params = DidChangeTextDocumentParams { + text_document: VersionedTextDocumentIdentifier { + uri: Url::from_file_path(path.as_ref()).expect("Path must be a valid URL"), version: self.next_document_version(), }, content_changes: changes, }; - self.send_notification::(params) + self.send_notification::(params) } /// Send a `textDocument/didClose` notification #[expect(dead_code)] - pub(crate) fn close_text_document(&mut self, uri: Url) -> Result<()> { - let params = lsp_types::DidCloseTextDocumentParams { - text_document: lsp_types::TextDocumentIdentifier { uri }, + pub(crate) fn close_text_document(&mut self, path: impl AsRef) -> Result<()> { + let params = DidCloseTextDocumentParams { + text_document: TextDocumentIdentifier { + uri: Url::from_file_path(path.as_ref()).expect("Path must be a valid URL"), + }, }; - self.send_notification::(params) + self.send_notification::(params) } /// Send a `workspace/didChangeWatchedFiles` notification with the given file events #[expect(dead_code)] pub(crate) fn did_change_watched_files(&mut self, events: Vec) -> Result<()> { - let params = lsp_types::DidChangeWatchedFilesParams { changes: events }; - self.send_notification::(params) + let params = DidChangeWatchedFilesParams { changes: events }; + self.send_notification::(params) + } + + /// Send a `textDocument/diagnostic` request for the document at the given path. + pub(crate) fn document_diagnostic_request( + &mut self, + path: impl AsRef, + ) -> Result { + let uri = Url::from_file_path(path.as_ref()).expect("Path must be a valid URL"); + let params = DocumentDiagnosticParams { + text_document: TextDocumentIdentifier { uri }, + identifier: Some("ty".to_string()), + previous_result_id: None, + work_done_progress_params: WorkDoneProgressParams::default(), + partial_result_params: PartialResultParams::default(), + }; + let id = self.send_request::(params)?; + self.get_response::(id) } } @@ -581,7 +610,6 @@ impl TestServerBuilder { } /// Enable or disable pull diagnostics capability - #[expect(dead_code)] pub(crate) fn with_pull_diagnostics(mut self, enabled: bool) -> Self { self.client_capabilities .text_document From c9be67b34c841455ed6be30c3ef13c24aa57756e Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Fri, 18 Jul 2025 11:15:20 +0530 Subject: [PATCH 03/13] Assert no pending messages before we drop the test server --- crates/ty_server/src/test.rs | 122 +++++++++++++++++++++++++++-------- 1 file changed, 96 insertions(+), 26 deletions(-) diff --git a/crates/ty_server/src/test.rs b/crates/ty_server/src/test.rs index d0883ac3f4cdec..b07beb5d2a92b5 100644 --- a/crates/ty_server/src/test.rs +++ b/crates/ty_server/src/test.rs @@ -1,7 +1,7 @@ //! Testing server for the ty language server. //! -//! This module provides mock server infrastructure for testing LSP functionality -//! without requiring actual file system operations or network connections. +//! This module provides mock server infrastructure for testing LSP functionality without requiring +//! actual file system operations. //! //! The design is inspired by the Starlark LSP test server but adapted for ty server architecture. @@ -14,6 +14,7 @@ use std::thread::JoinHandle; use std::time::Duration; use anyhow::Result; +use crossbeam::channel::RecvTimeoutError; use lsp_server::{Connection, Message, RequestId, Response, ResponseError}; use lsp_types::notification::{ DidChangeTextDocument, DidChangeWatchedFiles, DidCloseTextDocument, DidOpenTextDocument, Exit, @@ -53,10 +54,16 @@ pub(crate) enum TestServerError { #[error("Test client received an unrecognized request from the server: {0:?}")] UnrecognizedRequest(lsp_server::Request), + + #[error(transparent)] + RecvTimeoutError(#[from] RecvTimeoutError), } /// A test server for the ty language server that provides helpers for sending requests, /// correlating responses, and handling notifications. +/// +/// The [`Drop`] implementation ensures that the server is shut down gracefully using the described +/// protocol in the LSP specification. pub(crate) struct TestServer { /// The thread that's actually running the server server_thread: Option>, @@ -94,28 +101,34 @@ pub(crate) struct TestServer { impl Drop for TestServer { fn drop(&mut self) { + self.drain_messages(); + // Follow the LSP protocol to shutdown the server gracefully - match self.send_request::(()) { + let shutdown_error = match self.send_request::(()) { Ok(shutdown_id) => match self.get_response::<()>(shutdown_id) { Ok(()) => { if let Err(err) = self.send_notification::(()) { - panic!("Failed to send exit notification: {err:?}"); + Some(format!("Failed to send exit notification: {err:?}")) + } else { + None } } - Err(err) => { - panic!("Failed to get shutdown response: {err:?}"); - } + Err(err) => Some(format!("Failed to get shutdown response: {err:?}")), }, - Err(err) => { - panic!("Failed to send shutdown request: {err:?}"); - } - } + Err(err) => Some(format!("Failed to send shutdown request: {err:?}")), + }; if let Some(server_thread) = self.server_thread.take() { if let Err(err) = server_thread.join() { panic!("Test server thread did not join when dropped: {err:?}"); } } + + if let Some(error) = shutdown_error { + panic!("Test server did not shut down gracefully: {error}"); + } + + self.assert_no_pending_messages(); } } @@ -177,7 +190,7 @@ impl TestServer { responses: HashMap::new(), notifications: VecDeque::new(), requests: VecDeque::new(), - recv_timeout: Duration::from_secs(1), + recv_timeout: Duration::from_secs(2), initialize_response: None, workspace_configurations, registered_capabilities: Vec::new(), @@ -194,7 +207,8 @@ impl TestServer { let init_params = InitializeParams { capabilities, workspace_folders: Some(workspace_folders), - // TODO: This should be configurable by the test server builder + // TODO: This should be configurable by the test server builder. This might not be + // required after client settings are implemented in the server. initialization_options: Some(serde_json::Value::Object(serde_json::Map::new())), ..Default::default() }; @@ -218,6 +232,50 @@ impl TestServer { Ok(self) } + /// Drain all messages from the server. + fn drain_messages(&mut self) { + loop { + match self.receive() { + Ok(()) => {} + Err(TestServerError::RecvTimeoutError(_)) => { + // Only break if we have no more messages to process. + break; + } + Err(_) => {} + } + } + } + + /// Validate that there are no pending messages from the server. + /// + /// This should be called before the test server is dropped to ensure that all server messages + /// have been properly consumed by the test. If there are any pending messages, this will panic + /// with detailed information about what was left unconsumed. + fn assert_no_pending_messages(&self) { + let mut errors = Vec::new(); + + if !self.responses.is_empty() { + errors.push(format!("Unclaimed responses: {:#?}", self.responses)); + } + + if !self.notifications.is_empty() { + errors.push(format!( + "Unclaimed notifications: {:#?}", + self.notifications + )); + } + + if !self.requests.is_empty() { + errors.push(format!("Unclaimed requests: {:#?}", self.requests)); + } + + assert!( + errors.is_empty(), + "Test server has pending messages that were not consumed by the test:\n{}", + errors.join("\n") + ); + } + /// Generate a new request ID fn next_request_id(&mut self) -> RequestId { self.request_counter += 1; @@ -261,31 +319,35 @@ impl TestServer { /// Get a server response for the given request ID. /// - /// The request should have already been sent using [`send_request`]. + /// This should only be called if a request was already sent to the server via [`send_request`] + /// which returns the request ID that should be used here. + /// + /// This method will remove the response from the internal data structure, so it can only be + /// called once per request ID. /// /// [`send_request`]: TestServer::send_request pub(crate) fn get_response(&mut self, id: RequestId) -> Result { loop { self.receive()?; - if let Some(response) = self.responses.get(&id) { + if let Some(response) = self.responses.remove(&id) { match response { Response { error: None, result: Some(result), .. } => { - return Ok(serde_json::from_value::(result.clone())?); + return Ok(serde_json::from_value::(result)?); } Response { error: Some(err), result: None, .. } => { - return Err(TestServerError::ResponseError(err.clone()).into()); + return Err(TestServerError::ResponseError(err).into()); } response => { - return Err(TestServerError::InvalidResponse(id, response.clone()).into()); + return Err(TestServerError::InvalidResponse(id, response).into()); } } } @@ -295,8 +357,12 @@ impl TestServer { /// Get a notification of the specified type from the server and return its parameters. /// /// The caller should ensure that the server is expected to send this notification type. It - /// will keep polling the server for notifications up to 10 times before giving up. It can - /// return an error if the notification is not received within `recv_timeout` duration. + /// will keep polling the server for this notification up to 10 times before giving up after + /// which it will return an error. It will also return an error if the notification is not + /// received within `recv_timeout` duration. + /// + /// This method will remove the notification from the internal data structure, so it should + /// only be called if the notification is expected to be sent by the server. pub(crate) fn get_notification(&mut self) -> Result { for _ in 0..10 { self.receive()?; @@ -326,8 +392,12 @@ impl TestServer { /// parameters. /// /// The caller should ensure that the server is expected to send this request type. It will - /// keep polling the server for requests up to 10 times before giving up. It can return an - /// error if the request is not received within `recv_timeout` duration. + /// keep polling the server for this request up to 10 times before giving up after which it + /// will return an error. It can also return an error if the request is not received within + /// `recv_timeout` duration. + /// + /// This method will remove the request from the internal data structure, so it should only be + /// called if the request is expected to be sent by the server. pub(crate) fn get_request(&mut self) -> Result<(RequestId, R::Params)> { for _ in 0..10 { self.receive()?; @@ -363,7 +433,8 @@ impl TestServer { /// - Requests are stored in `requests` /// - Responses are stored in `responses` /// - Notifications are stored in `notifications` - fn receive(&mut self) -> Result<()> { + #[allow(clippy::result_large_err)] + fn receive(&mut self) -> Result<(), TestServerError> { let message = self .client_connection .receiver @@ -378,8 +449,7 @@ impl TestServer { return Err(TestServerError::DuplicateResponse( response.id, existing.get().clone(), - ) - .into()); + )); } Entry::Vacant(entry) => { entry.insert(response); @@ -417,7 +487,7 @@ impl TestServer { // TODO: Handle `python` section once it's implemented in the server // As per the spec: // - // > If the client can’t provide a configuration setting for a given scope + // > If the client can't provide a configuration setting for a given scope // > then null needs to be present in the returned array. serde_json::Value::Null } From a7aada5ee7426039e114f49265e4d4c25f8239c1 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Mon, 21 Jul 2025 14:36:15 +0530 Subject: [PATCH 04/13] Address review feedback --- crates/ty_server/src/server.rs | 40 +- crates/ty_server/src/session.rs | 14 +- ...erver__server__tests__initialization.snap} | 0 crates/ty_server/src/system.rs | 40 +- crates/ty_server/src/test.rs | 399 +++++++++--------- 5 files changed, 250 insertions(+), 243 deletions(-) rename crates/ty_server/src/snapshots/{ty_server__server__tests__initialization_capabilities.snap => ty_server__server__tests__initialization.snap} (100%) diff --git a/crates/ty_server/src/server.rs b/crates/ty_server/src/server.rs index 98f2368aa7dda2..8f3f512755b46a 100644 --- a/crates/ty_server/src/server.rs +++ b/crates/ty_server/src/server.rs @@ -39,7 +39,7 @@ impl Server { pub(crate) fn new( worker_threads: NonZeroUsize, connection: Connection, - fallback_system: Arc, + native_system: Arc, ) -> crate::Result { let (id, init_value) = connection.initialize_start()?; let init_params: InitializeParams = serde_json::from_value(init_value)?; @@ -107,7 +107,7 @@ impl Server { .collect() }) .or_else(|| { - let current_dir = fallback_system + let current_dir = native_system .current_directory() .as_std_path() .to_path_buf(); @@ -152,7 +152,7 @@ impl Server { position_encoding, global_options, workspaces, - fallback_system, + native_system, )?, client_capabilities, }) @@ -309,35 +309,33 @@ mod tests { use crate::test::TestServerBuilder; #[test] - fn initialization_sequence() { - let system = InMemorySystem::default(); + fn initialization() -> Result<()> { let test_server = TestServerBuilder::new() - .with_memory_system(system) - .build() - .unwrap() - .wait_until_workspaces_are_initialized() - .unwrap(); + .build()? + .wait_until_workspaces_are_initialized()?; let initialization_result = test_server.initialization_result().unwrap(); - insta::assert_json_snapshot!("initialization_capabilities", initialization_result); + insta::assert_json_snapshot!("initialization", initialization_result); + + Ok(()) } #[test] - fn initialization_with_workspace() { + fn initialization_with_workspace() -> Result<()> { let workspace_root = SystemPathBuf::from("/foo"); let system = InMemorySystem::new(workspace_root.clone()); let test_server = TestServerBuilder::new() .with_memory_system(system) .with_workspace(&workspace_root, ClientOptions::default()) - .build() - .unwrap() - .wait_until_workspaces_are_initialized() - .unwrap(); + .build()? + .wait_until_workspaces_are_initialized()?; let initialization_result = test_server.initialization_result().unwrap(); insta::assert_json_snapshot!("initialization_with_workspace", initialization_result); + + Ok(()) } #[test] @@ -355,12 +353,12 @@ def foo() -> str: let mut server = TestServerBuilder::new() .with_memory_system(InMemorySystem::from_memory_fs(fs)) .with_workspace(&workspace_root, ClientOptions::default()) - .with_pull_diagnostics(false) + .enable_pull_diagnostics(false) .build()? .wait_until_workspaces_are_initialized()?; - server.open_text_document(&foo, &foo_content)?; - let diagnostics = server.get_notification::()?; + server.open_text_document(&foo, &foo_content); + let diagnostics = server.await_notification::()?; insta::assert_debug_snapshot!(diagnostics); @@ -382,11 +380,11 @@ def foo() -> str: let mut server = TestServerBuilder::new() .with_memory_system(InMemorySystem::from_memory_fs(fs)) .with_workspace(&workspace_root, ClientOptions::default()) - .with_pull_diagnostics(true) + .enable_pull_diagnostics(true) .build()? .wait_until_workspaces_are_initialized()?; - server.open_text_document(&foo, &foo_content)?; + server.open_text_document(&foo, &foo_content); let diagnostics = server.document_diagnostic_request(&foo)?; insta::assert_debug_snapshot!(diagnostics); diff --git a/crates/ty_server/src/session.rs b/crates/ty_server/src/session.rs index 6072390f821455..a78d219a9e4660 100644 --- a/crates/ty_server/src/session.rs +++ b/crates/ty_server/src/session.rs @@ -40,8 +40,8 @@ mod settings; /// The global state for the LSP pub(crate) struct Session { - /// A fallback system to use with the [`LSPSystem`]. - fallback_system: Arc, + /// A native system to use with the [`LSPSystem`]. + native_system: Arc, /// Used to retrieve information about open documents and settings. /// @@ -105,7 +105,7 @@ impl Session { position_encoding: PositionEncoding, global_options: GlobalOptions, workspace_folders: Vec<(Url, ClientOptions)>, - fallback_system: Arc, + native_system: Arc, ) -> crate::Result { let index = Arc::new(Index::new(global_options.into_settings())); @@ -115,7 +115,7 @@ impl Session { } Ok(Self { - fallback_system, + native_system, position_encoding, workspaces, deferred_messages: VecDeque::new(), @@ -235,7 +235,7 @@ impl Session { AnySystemPath::System(system_path) => { self.project_state_for_path(system_path).unwrap_or_else(|| { self.default_project - .get(self.index.as_ref(), &self.fallback_system) + .get(self.index.as_ref(), &self.native_system) }) } AnySystemPath::SystemVirtual(_virtual_path) => { @@ -266,7 +266,7 @@ impl Session { .map(|(_, project)| project) .unwrap_or_else(|| { self.default_project - .get_mut(self.index.as_ref(), &self.fallback_system) + .get_mut(self.index.as_ref(), &self.native_system) }), AnySystemPath::SystemVirtual(_virtual_path) => { // TODO: Currently, ty only supports single workspace but we need to figure out @@ -352,7 +352,7 @@ impl Session { // and create a project database for each. let system = LSPSystem::new( self.index.as_ref().unwrap().clone(), - self.fallback_system.clone(), + self.native_system.clone(), ); let project = ProjectMetadata::discover(&root, &system) diff --git a/crates/ty_server/src/snapshots/ty_server__server__tests__initialization_capabilities.snap b/crates/ty_server/src/snapshots/ty_server__server__tests__initialization.snap similarity index 100% rename from crates/ty_server/src/snapshots/ty_server__server__tests__initialization_capabilities.snap rename to crates/ty_server/src/snapshots/ty_server__server__tests__initialization.snap diff --git a/crates/ty_server/src/system.rs b/crates/ty_server/src/system.rs index c8ef6369ae7127..7f5ad0cdfc3555 100644 --- a/crates/ty_server/src/system.rs +++ b/crates/ty_server/src/system.rs @@ -119,18 +119,21 @@ pub(crate) struct LSPSystem { /// [`index_mut`]: crate::Session::index_mut index: Option>, - /// A fallback system implementation used when documents are not found in the index. - fallback_system: Arc, + /// A native system implementation. + /// + /// This is used to delegate method calls that are not handled by the LSP system. It is also + /// used as a fallback when the documents are not found in the LSP index. + native_system: Arc, } impl LSPSystem { pub(crate) fn new( index: Arc, - fallback_system: Arc, + native_system: Arc, ) -> Self { Self { index: Some(index), - fallback_system, + native_system, } } @@ -184,17 +187,16 @@ impl System for LSPSystem { FileType::File, )) } else { - self.fallback_system.path_metadata(path) + self.native_system.path_metadata(path) } } fn canonicalize_path(&self, path: &SystemPath) -> Result { - self.fallback_system.canonicalize_path(path) + self.native_system.canonicalize_path(path) } fn path_exists_case_sensitive(&self, path: &SystemPath, prefix: &SystemPath) -> bool { - self.fallback_system - .path_exists_case_sensitive(path, prefix) + self.native_system.path_exists_case_sensitive(path, prefix) } fn read_to_string(&self, path: &SystemPath) -> Result { @@ -202,7 +204,7 @@ impl System for LSPSystem { match document { Some(DocumentQuery::Text { document, .. }) => Ok(document.contents().to_string()), - _ => self.fallback_system.read_to_string(path), + _ => self.native_system.read_to_string(path), } } @@ -214,7 +216,7 @@ impl System for LSPSystem { Notebook::from_source_code(document.contents()) } Some(DocumentQuery::Notebook { notebook, .. }) => Ok(notebook.make_ruff_notebook()), - None => self.fallback_system.read_to_notebook(path), + None => self.native_system.read_to_notebook(path), } } @@ -245,26 +247,26 @@ impl System for LSPSystem { } fn current_directory(&self) -> &SystemPath { - self.fallback_system.current_directory() + self.native_system.current_directory() } fn user_config_directory(&self) -> Option { - self.fallback_system.user_config_directory() + self.native_system.user_config_directory() } fn cache_dir(&self) -> Option { - self.fallback_system.cache_dir() + self.native_system.cache_dir() } fn read_directory<'a>( &'a self, path: &SystemPath, ) -> Result> + 'a>> { - self.fallback_system.read_directory(path) + self.native_system.read_directory(path) } fn walk_directory(&self, path: &SystemPath) -> WalkDirectoryBuilder { - self.fallback_system.walk_directory(path) + self.native_system.walk_directory(path) } fn glob( @@ -274,11 +276,11 @@ impl System for LSPSystem { Box> + '_>, PatternError, > { - self.fallback_system.glob(pattern) + self.native_system.glob(pattern) } fn as_writable(&self) -> Option<&dyn WritableSystem> { - self.fallback_system.as_writable() + self.native_system.as_writable() } fn as_any(&self) -> &dyn Any { @@ -290,11 +292,11 @@ impl System for LSPSystem { } fn case_sensitivity(&self) -> CaseSensitivity { - self.fallback_system.case_sensitivity() + self.native_system.case_sensitivity() } fn env_var(&self, name: &str) -> std::result::Result { - self.fallback_system.env_var(name) + self.native_system.env_var(name) } } diff --git a/crates/ty_server/src/test.rs b/crates/ty_server/src/test.rs index b07beb5d2a92b5..e6887e15a20c33 100644 --- a/crates/ty_server/src/test.rs +++ b/crates/ty_server/src/test.rs @@ -29,9 +29,9 @@ use lsp_types::{ DidChangeWatchedFilesParams, DidCloseTextDocumentParams, DidOpenTextDocumentParams, DocumentDiagnosticParams, DocumentDiagnosticReportResult, FileEvent, InitializeParams, InitializeResult, InitializedParams, PartialResultParams, PublishDiagnosticsClientCapabilities, - RegistrationParams, TextDocumentClientCapabilities, TextDocumentContentChangeEvent, - TextDocumentIdentifier, TextDocumentItem, Url, VersionedTextDocumentIdentifier, - WorkDoneProgressParams, WorkspaceClientCapabilities, WorkspaceFolder, + TextDocumentClientCapabilities, TextDocumentContentChangeEvent, TextDocumentIdentifier, + TextDocumentItem, Url, VersionedTextDocumentIdentifier, WorkDoneProgressParams, + WorkspaceClientCapabilities, WorkspaceFolder, }; use ruff_db::system::{InMemorySystem, SystemPath, TestSystem}; use serde::de::DeserializeOwned; @@ -39,6 +39,9 @@ use serde::de::DeserializeOwned; use crate::server::Server; use crate::session::ClientOptions; +/// Number of times to retry receiving a message before giving up +const RETRY_COUNT: usize = 5; + /// Errors that can occur during testing #[derive(thiserror::Error, Debug)] pub(crate) enum TestServerError { @@ -52,24 +55,28 @@ pub(crate) enum TestServerError { #[error("Got a duplicate response for request ID {0}: {1:?}")] DuplicateResponse(RequestId, Response), - #[error("Test client received an unrecognized request from the server: {0:?}")] - UnrecognizedRequest(lsp_server::Request), - - #[error(transparent)] - RecvTimeoutError(#[from] RecvTimeoutError), + #[error("Timeout while waiting for a message from the server")] + RecvTimeoutError, } /// A test server for the ty language server that provides helpers for sending requests, /// correlating responses, and handling notifications. /// /// The [`Drop`] implementation ensures that the server is shut down gracefully using the described -/// protocol in the LSP specification. +/// protocol in the LSP specification. It also ensures that all messages sent by the server have +/// been handled by the test client before the server is dropped. pub(crate) struct TestServer { - /// The thread that's actually running the server + /// The thread that's actually running the server. + /// + /// This is an [`Option`] so that the join handle can be taken out when the server is dropped, + /// allowing the server thread to be joined and cleaned up properly. server_thread: Option>, - /// Connection to communicate with the server - client_connection: Connection, + /// Connection to communicate with the server. + /// + /// This is an [`Option`] so that it can be taken out when the server is dropped, allowing + /// the connection to be cleaned up properly. + client_connection: Option, /// Incrementing counter to automatically generate request IDs request_counter: i32, @@ -86,9 +93,6 @@ pub(crate) struct TestServer { /// An ordered queue of all the requests received from the server requests: VecDeque, - /// How long to wait for messages to be received - recv_timeout: Duration, - /// The response from server initialization initialize_response: Option, @@ -99,69 +103,13 @@ pub(crate) struct TestServer { registered_capabilities: Vec, } -impl Drop for TestServer { - fn drop(&mut self) { - self.drain_messages(); - - // Follow the LSP protocol to shutdown the server gracefully - let shutdown_error = match self.send_request::(()) { - Ok(shutdown_id) => match self.get_response::<()>(shutdown_id) { - Ok(()) => { - if let Err(err) = self.send_notification::(()) { - Some(format!("Failed to send exit notification: {err:?}")) - } else { - None - } - } - Err(err) => Some(format!("Failed to get shutdown response: {err:?}")), - }, - Err(err) => Some(format!("Failed to send shutdown request: {err:?}")), - }; - - if let Some(server_thread) = self.server_thread.take() { - if let Err(err) = server_thread.join() { - panic!("Test server thread did not join when dropped: {err:?}"); - } - } - - if let Some(error) = shutdown_error { - panic!("Test server did not shut down gracefully: {error}"); - } - - self.assert_no_pending_messages(); - } -} - -impl fmt::Debug for TestServer { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("TestServer") - .field("request_counter", &self.request_counter) - .field("version_counter", &self.version_counter) - .field("responses", &self.responses) - .field("notifications", &self.notifications) - .field("server_requests", &self.requests) - .field("recv_timeout", &self.recv_timeout) - .field("initialize_response", &self.initialize_response) - .field("workspace_configurations", &self.workspace_configurations) - .field("registered_capabilities", &self.registered_capabilities) - .finish_non_exhaustive() - } -} - impl TestServer { /// Create a new test server with the given workspace configurations pub(crate) fn new( - workspace_folders: Vec, - workspace_configurations: HashMap, + workspaces: Vec<(WorkspaceFolder, ClientOptions)>, memory_system: InMemorySystem, capabilities: ClientCapabilities, ) -> Result { - assert_eq!( - workspace_folders.len(), - workspace_configurations.len(), - "Number of workspace folders should match the number of workspace configurations" - ); - let (server_connection, client_connection) = Connection::memory(); // Start the server in a separate thread @@ -182,15 +130,24 @@ impl TestServer { } }); + let workspace_folders = workspaces + .iter() + .map(|(folder, _)| folder.clone()) + .collect::>(); + + let workspace_configurations = workspaces + .into_iter() + .map(|(folder, options)| (folder.uri, options)) + .collect::>(); + Self { server_thread: Some(server_thread), - client_connection, + client_connection: Some(client_connection), request_counter: 0, version_counter: 0, responses: HashMap::new(), notifications: VecDeque::new(), requests: VecDeque::new(), - recv_timeout: Duration::from_secs(2), initialize_response: None, workspace_configurations, registered_capabilities: Vec::new(), @@ -213,9 +170,9 @@ impl TestServer { ..Default::default() }; - let init_request_id = self.send_request::(init_params)?; - self.initialize_response = Some(self.get_response::(init_request_id)?); - self.send_notification::(InitializedParams {})?; + let init_request_id = self.send_request::(init_params); + self.initialize_response = Some(self.await_response::(init_request_id)?); + self.send_notification::(InitializedParams {}); Ok(self) } @@ -227,7 +184,7 @@ impl TestServer { /// /// This should only be called if the server is expected to send this request. pub(crate) fn wait_until_workspaces_are_initialized(mut self) -> Result { - let (request_id, params) = self.get_request::()?; + let (request_id, params) = self.await_request::()?; self.handle_workspace_configuration_request(request_id, ¶ms)?; Ok(self) } @@ -235,9 +192,11 @@ impl TestServer { /// Drain all messages from the server. fn drain_messages(&mut self) { loop { - match self.receive() { + // Don't wait too long to drain the messages, as this is called in the `Drop` + // implementation which happens everytime the test ends. + match self.receive(Some(Duration::from_millis(10))) { Ok(()) => {} - Err(TestServerError::RecvTimeoutError(_)) => { + Err(TestServerError::RecvTimeoutError) => { // Only break if we have no more messages to process. break; } @@ -288,36 +247,51 @@ impl TestServer { self.version_counter } + /// Send a message to the server. + /// + /// # Panics + /// + /// If the server is still running but the client connection got dropped, or if the server + /// exited unexpectedly or panicked. + #[track_caller] + fn send(&mut self, message: Message) { + if self + .client_connection + .as_ref() + .unwrap() + .sender + .send(message) + .is_err() + { + self.panic_on_server_disconnect(); + } + } + /// Send a request to the server and return the request ID. /// /// The caller can use this ID to later retrieve the response using [`get_response`]. /// /// [`get_response`]: TestServer::get_response - pub(crate) fn send_request(&mut self, params: R::Params) -> Result + pub(crate) fn send_request(&mut self, params: R::Params) -> RequestId where R: Request, { let id = self.next_request_id(); let request = lsp_server::Request::new(id.clone(), R::METHOD.to_string(), params); - self.client_connection - .sender - .send(Message::Request(request))?; - Ok(id) + self.send(Message::Request(request)); + id } /// Send a notification to the server. - pub(crate) fn send_notification(&self, params: N::Params) -> Result<()> + pub(crate) fn send_notification(&mut self, params: N::Params) where N: Notification, { let notification = lsp_server::Notification::new(N::METHOD.to_string(), params); - self.client_connection - .sender - .send(Message::Notification(notification))?; - Ok(()) + self.send(Message::Notification(notification)); } - /// Get a server response for the given request ID. + /// Wait for a server response corresponding to the given request ID. /// /// This should only be called if a request was already sent to the server via [`send_request`] /// which returns the request ID that should be used here. @@ -326,9 +300,9 @@ impl TestServer { /// called once per request ID. /// /// [`send_request`]: TestServer::send_request - pub(crate) fn get_response(&mut self, id: RequestId) -> Result { + pub(crate) fn await_response(&mut self, id: RequestId) -> Result { loop { - self.receive()?; + self.receive(None)?; if let Some(response) = self.responses.remove(&id) { match response { @@ -354,7 +328,7 @@ impl TestServer { } } - /// Get a notification of the specified type from the server and return its parameters. + /// Wait for a notification of the specified type from the server and return its parameters. /// /// The caller should ensure that the server is expected to send this notification type. It /// will keep polling the server for this notification up to 10 times before giving up after @@ -363,32 +337,26 @@ impl TestServer { /// /// This method will remove the notification from the internal data structure, so it should /// only be called if the notification is expected to be sent by the server. - pub(crate) fn get_notification(&mut self) -> Result { - for _ in 0..10 { - self.receive()?; + pub(crate) fn await_notification(&mut self) -> Result { + for _ in 0..RETRY_COUNT { + self.receive(None)?; let notification = self .notifications .iter() - .enumerate() - .find_map(|(index, notification)| { - if N::METHOD == notification.method { - Some(index) - } else { - None - } - }) + .position(|notification| N::METHOD == notification.method) .and_then(|index| self.notifications.remove(index)); if let Some(notification) = notification { return Ok(serde_json::from_value(notification.params)?); } + tracing::info!("Retrying to receive `{}` notification", N::METHOD); } Err(anyhow::anyhow!( - "Did not get a notification of type `{}` in 10 retries", + "Failed to receive `{}` notification after {RETRY_COUNT} retries", N::METHOD )) } - /// Get a request of the specified type from the server and return the request ID and + /// Wait for a request of the specified type from the server and return the request ID and /// parameters. /// /// The caller should ensure that the server is expected to send this request type. It will @@ -398,48 +366,59 @@ impl TestServer { /// /// This method will remove the request from the internal data structure, so it should only be /// called if the request is expected to be sent by the server. - pub(crate) fn get_request(&mut self) -> Result<(RequestId, R::Params)> { - for _ in 0..10 { - self.receive()?; + pub(crate) fn await_request(&mut self) -> Result<(RequestId, R::Params)> { + for _ in 0..RETRY_COUNT { + self.receive(None)?; let request = self .requests .iter() - .enumerate() - .find_map(|(index, request)| { - if R::METHOD == request.method { - Some(index) - } else { - None - } - }) + .position(|request| R::METHOD == request.method) .and_then(|index| self.requests.remove(index)); if let Some(request) = request { let params = serde_json::from_value(request.params)?; return Ok((request.id, params)); } + tracing::info!("Retrying to receive `{}` request", R::METHOD); } Err(anyhow::anyhow!( - "Did not get a request of type `{}` in 10 retries", + "Failed to receive `{}` request after {RETRY_COUNT} retries", R::METHOD )) } /// Receive a message from the server. /// - /// It will wait for `recv_timeout` duration for a message to arrive. If no message is received + /// It will wait for `timeout` duration for a message to arrive. If no message is received /// within that time, it will return an error. /// - /// Once a message is received, it will store it in the appropriate queue: - /// - Requests are stored in `requests` - /// - Responses are stored in `responses` - /// - Notifications are stored in `notifications` + /// If `timeout` is `None`, it will use a default timeout of 1 second. #[allow(clippy::result_large_err)] - fn receive(&mut self) -> Result<(), TestServerError> { - let message = self + fn receive(&mut self, timeout: Option) -> Result<(), TestServerError> { + static DEFAULT_TIMEOUT: Duration = Duration::from_secs(1); + + match self .client_connection + .as_ref() + .unwrap() .receiver - .recv_timeout(self.recv_timeout)?; + .recv_timeout(timeout.unwrap_or(DEFAULT_TIMEOUT)) + { + Ok(message) => self.handle_message(message), + Err(RecvTimeoutError::Timeout) => Err(TestServerError::RecvTimeoutError), + Err(RecvTimeoutError::Disconnected) => { + self.panic_on_server_disconnect(); + } + } + } + /// Handle the incoming message from the server. + /// + /// This method will store the message as follows: + /// - Requests are stored in `self.requests` + /// - Responses are stored in `self.responses` with the request ID as the key + /// - Notifications are stored in `self.notifications` + #[allow(clippy::result_large_err)] + fn handle_message(&mut self, message: Message) -> Result<(), TestServerError> { match message { Message::Request(request) => { self.requests.push_back(request); @@ -459,10 +438,24 @@ impl TestServer { self.notifications.push_back(notification); } } - Ok(()) } + #[track_caller] + fn panic_on_server_disconnect(&mut self) -> ! { + if let Some(handle) = &self.server_thread { + if handle.is_finished() { + let handle = self.server_thread.take().unwrap(); + if let Err(panic) = handle.join() { + std::panic::resume_unwind(panic); + } + panic!("Server exited unexpectedly"); + } + } + + panic!("Server dropped client receiver while still running"); + } + /// Handle workspace configuration requests from the server. /// /// Use the [`get_request`] method to wait for the server to send this request. @@ -493,54 +486,14 @@ impl TestServer { } } } else { - // TODO: Should we return an error? User should explicitly configure the test - // server to have a configuration for all workspaces even if they are empty. + tracing::warn!("No workspace configuration found for {scope_uri}"); serde_json::Value::Null }; results.push(config_value); } let response = Response::new_ok(request_id, results); - self.client_connection - .sender - .send(Message::Response(response))?; - - Ok(()) - } - - /// Handle requests from the server (like configuration requests) - #[expect(dead_code)] - fn handle_server_request(&mut self, request: lsp_server::Request) -> Result<()> { - match request.method.as_str() { - "workspace/configuration" => { - let params: ConfigurationParams = serde_json::from_value(request.params)?; - self.handle_workspace_configuration_request(request.id, ¶ms)?; - } - "workspace/diagnostic/refresh" => { - // TODO: Send diagnostic requests for all the open files to the server and send the - // workspace diagnostics request if the server supports it. - let response = Response::new_ok(request.id, serde_json::Value::Null); - self.client_connection - .sender - .send(Message::Response(response))?; - } - "client/registerCapability" => { - // TODO: We might have to expand this to handle more complex registrations and also - // handle unregistration. - let params: RegistrationParams = serde_json::from_value(request.params)?; - for registration in params.registrations { - self.registered_capabilities.push(registration.method); - } - // Accept capability registration requests - let response = Response::new_ok(request.id, serde_json::Value::Null); - self.client_connection - .sender - .send(Message::Response(response))?; - } - _ => { - return Err(TestServerError::UnrecognizedRequest(request).into()); - } - } + self.send(Message::Response(response)); Ok(()) } @@ -555,7 +508,7 @@ impl TestServer { &mut self, path: impl AsRef, content: &impl ToString, - ) -> Result<()> { + ) { let params = DidOpenTextDocumentParams { text_document: TextDocumentItem { uri: Url::from_file_path(path.as_ref()).expect("Path must be a valid URL"), @@ -564,7 +517,7 @@ impl TestServer { text: content.to_string(), }, }; - self.send_notification::(params) + self.send_notification::(params); } /// Send a `textDocument/didChange` notification with the given content changes @@ -573,7 +526,7 @@ impl TestServer { &mut self, path: impl AsRef, changes: Vec, - ) -> Result<()> { + ) { let params = DidChangeTextDocumentParams { text_document: VersionedTextDocumentIdentifier { uri: Url::from_file_path(path.as_ref()).expect("Path must be a valid URL"), @@ -581,25 +534,25 @@ impl TestServer { }, content_changes: changes, }; - self.send_notification::(params) + self.send_notification::(params); } /// Send a `textDocument/didClose` notification #[expect(dead_code)] - pub(crate) fn close_text_document(&mut self, path: impl AsRef) -> Result<()> { + pub(crate) fn close_text_document(&mut self, path: impl AsRef) { let params = DidCloseTextDocumentParams { text_document: TextDocumentIdentifier { uri: Url::from_file_path(path.as_ref()).expect("Path must be a valid URL"), }, }; - self.send_notification::(params) + self.send_notification::(params); } /// Send a `workspace/didChangeWatchedFiles` notification with the given file events #[expect(dead_code)] - pub(crate) fn did_change_watched_files(&mut self, events: Vec) -> Result<()> { + pub(crate) fn did_change_watched_files(&mut self, events: Vec) { let params = DidChangeWatchedFilesParams { changes: events }; - self.send_notification::(params) + self.send_notification::(params); } /// Send a `textDocument/diagnostic` request for the document at the given path. @@ -615,15 +568,71 @@ impl TestServer { work_done_progress_params: WorkDoneProgressParams::default(), partial_result_params: PartialResultParams::default(), }; - let id = self.send_request::(params)?; - self.get_response::(id) + let id = self.send_request::(params); + self.await_response::(id) + } +} + +impl fmt::Debug for TestServer { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("TestServer") + .field("request_counter", &self.request_counter) + .field("version_counter", &self.version_counter) + .field("responses", &self.responses) + .field("notifications", &self.notifications) + .field("server_requests", &self.requests) + .field("initialize_response", &self.initialize_response) + .field("workspace_configurations", &self.workspace_configurations) + .field("registered_capabilities", &self.registered_capabilities) + .finish_non_exhaustive() + } +} + +impl Drop for TestServer { + fn drop(&mut self) { + self.drain_messages(); + + // Follow the LSP protocol to shutdown the server gracefully. + // + // The `server_thread` could be `None` if the server exited unexpectedly or panicked or if + // it dropped the client connection. + let shutdown_error = self + .server_thread + .is_some() + .then(|| { + let shutdown_id = self.send_request::(()); + match self.await_response::<()>(shutdown_id) { + Ok(()) => { + self.send_notification::(()); + None + } + Err(err) => Some(format!("Failed to get shutdown response: {err:?}")), + } + }) + .flatten(); + + if let Some(_client_connection) = self.client_connection.take() { + // Drop the client connection before joining the server thread to avoid any hangs + // in case the server didn't respond to the shutdown request. + } + + if let Some(server_thread) = self.server_thread.take() { + if let Err(err) = server_thread.join() { + panic!("Panic in the server thread: {err:?}"); + } + } + + if let Some(error) = shutdown_error { + panic!("Test server did not shut down gracefully: {error}"); + } + + self.assert_no_pending_messages(); } } /// Builder for creating test servers with specific configurations pub(crate) struct TestServerBuilder { - workspace_folders: Vec, - workspace_configurations: HashMap, + workspaces: Vec<(WorkspaceFolder, ClientOptions)>, memory_system: InMemorySystem, client_capabilities: ClientCapabilities, } @@ -649,8 +658,7 @@ impl TestServerBuilder { }; Self { - workspace_folders: Vec::new(), - workspace_configurations: HashMap::new(), + workspaces: Vec::new(), memory_system: InMemorySystem::default(), client_capabilities, } @@ -662,14 +670,14 @@ impl TestServerBuilder { workspace_root: &SystemPath, options: ClientOptions, ) -> Self { - let workspace_folder = WorkspaceFolder { - uri: Url::from_file_path(workspace_root.as_std_path()) - .expect("workspace root must be a valid URL"), - name: workspace_root.file_name().unwrap_or("test").to_string(), - }; - self.workspace_configurations - .insert(workspace_folder.uri.clone(), options); - self.workspace_folders.push(workspace_folder); + self.workspaces.push(( + WorkspaceFolder { + uri: Url::from_file_path(workspace_root.as_std_path()) + .expect("workspace root must be a valid URL"), + name: workspace_root.file_name().unwrap_or("test").to_string(), + }, + options, + )); self } @@ -680,7 +688,7 @@ impl TestServerBuilder { } /// Enable or disable pull diagnostics capability - pub(crate) fn with_pull_diagnostics(mut self, enabled: bool) -> Self { + pub(crate) fn enable_pull_diagnostics(mut self, enabled: bool) -> Self { self.client_capabilities .text_document .get_or_insert_with(Default::default) @@ -694,7 +702,7 @@ impl TestServerBuilder { /// Enable or disable file watching capability #[expect(dead_code)] - pub(crate) fn with_did_change_watched_files(mut self, enabled: bool) -> Self { + pub(crate) fn enable_did_change_watched_files(mut self, enabled: bool) -> Self { self.client_capabilities .workspace .get_or_insert_with(Default::default) @@ -716,8 +724,7 @@ impl TestServerBuilder { /// Build the test server pub(crate) fn build(self) -> Result { TestServer::new( - self.workspace_folders, - self.workspace_configurations, + self.workspaces, self.memory_system, self.client_capabilities, ) From 6d9466e626c0dde2dfaf920ebada076e3ed16f8b Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Mon, 21 Jul 2025 14:48:56 +0530 Subject: [PATCH 05/13] Avoid assertion if test server is panicking --- crates/ty_server/src/test.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/ty_server/src/test.rs b/crates/ty_server/src/test.rs index e6887e15a20c33..be0a75ad9be1f4 100644 --- a/crates/ty_server/src/test.rs +++ b/crates/ty_server/src/test.rs @@ -622,6 +622,11 @@ impl Drop for TestServer { } } + if std::thread::panicking() { + // If the test server panicked, avoid further assertions. + return; + } + if let Some(error) = shutdown_error { panic!("Test server did not shut down gracefully: {error}"); } From a1068f6f0fe0e3b35f748ab86f394b3c1d9c6b23 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Mon, 21 Jul 2025 21:30:53 +0530 Subject: [PATCH 06/13] Use tempdir instead of memory file system --- Cargo.lock | 1 + crates/ty_server/Cargo.toml | 1 + crates/ty_server/src/server.rs | 36 ++++++---------- crates/ty_server/src/test.rs | 79 +++++++++++++++++++++++++--------- 4 files changed, 75 insertions(+), 42 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 46062d9a9ef130..dd32a9f400e88e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4305,6 +4305,7 @@ dependencies = [ "serde", "serde_json", "shellexpand", + "tempfile", "thiserror 2.0.12", "tracing", "tracing-subscriber", diff --git a/crates/ty_server/Cargo.toml b/crates/ty_server/Cargo.toml index 3b52524470d6af..7658dc715b8877 100644 --- a/crates/ty_server/Cargo.toml +++ b/crates/ty_server/Cargo.toml @@ -39,6 +39,7 @@ tracing-subscriber = { workspace = true, features = ["chrono"] } [dev-dependencies] insta = { workspace = true, features = ["json"] } +tempfile = { workspace = true } [target.'cfg(target_vendor = "apple")'.dependencies] libc = { workspace = true } diff --git a/crates/ty_server/src/server.rs b/crates/ty_server/src/server.rs index 8f3f512755b46a..fd51abc73049a9 100644 --- a/crates/ty_server/src/server.rs +++ b/crates/ty_server/src/server.rs @@ -303,7 +303,7 @@ impl Drop for ServerPanicHookHandler { mod tests { use anyhow::Result; use lsp_types::notification::PublishDiagnostics; - use ruff_db::system::{InMemorySystem, MemoryFileSystem, SystemPathBuf}; + use ruff_db::system::SystemPath; use crate::session::ClientOptions; use crate::test::TestServerBuilder; @@ -323,11 +323,9 @@ mod tests { #[test] fn initialization_with_workspace() -> Result<()> { - let workspace_root = SystemPathBuf::from("/foo"); - let system = InMemorySystem::new(workspace_root.clone()); + let workspace_root = SystemPath::new("foo"); let test_server = TestServerBuilder::new() - .with_memory_system(system) - .with_workspace(&workspace_root, ClientOptions::default()) + .with_workspace(workspace_root, ClientOptions::default()) .build()? .wait_until_workspaces_are_initialized()?; @@ -340,24 +338,21 @@ mod tests { #[test] fn publish_diagnostics_on_did_open() -> Result<()> { - let workspace_root = SystemPathBuf::from("/src"); - - let fs = MemoryFileSystem::with_current_directory(&workspace_root); - let foo = workspace_root.join("foo.py"); + let workspace_root = SystemPath::new("src"); + let foo = SystemPath::new("src/foo.py"); let foo_content = "\ def foo() -> str: return 42 "; - fs.write_file(&foo, foo_content)?; let mut server = TestServerBuilder::new() - .with_memory_system(InMemorySystem::from_memory_fs(fs)) - .with_workspace(&workspace_root, ClientOptions::default()) + .write_file(foo, foo_content)? + .with_workspace(workspace_root, ClientOptions::default()) .enable_pull_diagnostics(false) .build()? .wait_until_workspaces_are_initialized()?; - server.open_text_document(&foo, &foo_content); + server.open_text_document(foo, &foo_content); let diagnostics = server.await_notification::()?; insta::assert_debug_snapshot!(diagnostics); @@ -367,25 +362,22 @@ def foo() -> str: #[test] fn pull_diagnostics_on_did_open() -> Result<()> { - let workspace_root = SystemPathBuf::from("/src"); - - let fs = MemoryFileSystem::with_current_directory(&workspace_root); - let foo = workspace_root.join("foo.py"); + let workspace_root = SystemPath::new("src"); + let foo = SystemPath::new("src/foo.py"); let foo_content = "\ def foo() -> str: return 42 "; - fs.write_file(&foo, foo_content)?; let mut server = TestServerBuilder::new() - .with_memory_system(InMemorySystem::from_memory_fs(fs)) - .with_workspace(&workspace_root, ClientOptions::default()) + .write_file(foo, foo_content)? + .with_workspace(workspace_root, ClientOptions::default()) .enable_pull_diagnostics(true) .build()? .wait_until_workspaces_are_initialized()?; - server.open_text_document(&foo, &foo_content); - let diagnostics = server.document_diagnostic_request(&foo)?; + server.open_text_document(foo, &foo_content); + let diagnostics = server.document_diagnostic_request(foo)?; insta::assert_debug_snapshot!(diagnostics); diff --git a/crates/ty_server/src/test.rs b/crates/ty_server/src/test.rs index be0a75ad9be1f4..820f897c28de04 100644 --- a/crates/ty_server/src/test.rs +++ b/crates/ty_server/src/test.rs @@ -1,7 +1,7 @@ //! Testing server for the ty language server. //! -//! This module provides mock server infrastructure for testing LSP functionality without requiring -//! actual file system operations. +//! This module provides mock server infrastructure for testing LSP functionality using +//! temporary directories on the real filesystem. //! //! The design is inspired by the Starlark LSP test server but adapted for ty server architecture. @@ -33,8 +33,9 @@ use lsp_types::{ TextDocumentItem, Url, VersionedTextDocumentIdentifier, WorkDoneProgressParams, WorkspaceClientCapabilities, WorkspaceFolder, }; -use ruff_db::system::{InMemorySystem, SystemPath, TestSystem}; +use ruff_db::system::{OsSystem, SystemPath, TestSystem}; use serde::de::DeserializeOwned; +use tempfile::TempDir; use crate::server::Server; use crate::session::ClientOptions; @@ -78,6 +79,11 @@ pub(crate) struct TestServer { /// the connection to be cleaned up properly. client_connection: Option, + /// Temporary directory that holds all test files. + /// + /// This directory is automatically cleaned up when the [`TestServer`] is dropped. + temp_dir: TempDir, + /// Incrementing counter to automatically generate request IDs request_counter: i32, @@ -107,16 +113,20 @@ impl TestServer { /// Create a new test server with the given workspace configurations pub(crate) fn new( workspaces: Vec<(WorkspaceFolder, ClientOptions)>, - memory_system: InMemorySystem, + temp_dir: TempDir, capabilities: ClientCapabilities, ) -> Result { let (server_connection, client_connection) = Connection::memory(); + // Create OS system with the temp directory as cwd + let temp_path = SystemPath::from_std_path(temp_dir.path()).unwrap(); + let os_system = OsSystem::new(temp_path); + // Start the server in a separate thread let server_thread = std::thread::spawn(move || { // TODO: This should probably be configurable to test concurrency issues let worker_threads = NonZeroUsize::new(1).unwrap(); - let test_system = Arc::new(TestSystem::new(memory_system)); + let test_system = Arc::new(TestSystem::new(os_system)); match Server::new(worker_threads, server_connection, test_system) { Ok(server) => { @@ -143,6 +153,7 @@ impl TestServer { Self { server_thread: Some(server_thread), client_connection: Some(client_connection), + temp_dir, request_counter: 0, version_counter: 0, responses: HashMap::new(), @@ -576,6 +587,7 @@ impl TestServer { impl fmt::Debug for TestServer { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("TestServer") + .field("temp_dir", &self.temp_dir.path()) .field("request_counter", &self.request_counter) .field("version_counter", &self.version_counter) .field("responses", &self.responses) @@ -637,8 +649,8 @@ impl Drop for TestServer { /// Builder for creating test servers with specific configurations pub(crate) struct TestServerBuilder { + temp_dir: TempDir, workspaces: Vec<(WorkspaceFolder, ClientOptions)>, - memory_system: InMemorySystem, client_capabilities: ClientCapabilities, } @@ -664,7 +676,7 @@ impl TestServerBuilder { Self { workspaces: Vec::new(), - memory_system: InMemorySystem::default(), + temp_dir: TempDir::new().expect("should be able to create temporary directory"), client_capabilities, } } @@ -675,10 +687,13 @@ impl TestServerBuilder { workspace_root: &SystemPath, options: ClientOptions, ) -> Self { + let temp_system_path = SystemPath::from_std_path(self.temp_dir.path()).unwrap(); + let workspace_path = temp_system_path.join(workspace_root); + self.workspaces.push(( WorkspaceFolder { - uri: Url::from_file_path(workspace_root.as_std_path()) - .expect("workspace root must be a valid URL"), + uri: Url::from_file_path(workspace_path.as_std_path()) + .expect("workspace root should be a valid URL"), name: workspace_root.file_name().unwrap_or("test").to_string(), }, options, @@ -686,12 +701,6 @@ impl TestServerBuilder { self } - /// Set the in-memory system - pub(crate) fn with_memory_system(mut self, memory_system: InMemorySystem) -> Self { - self.memory_system = memory_system; - self - } - /// Enable or disable pull diagnostics capability pub(crate) fn enable_pull_diagnostics(mut self, enabled: bool) -> Self { self.client_capabilities @@ -726,12 +735,42 @@ impl TestServerBuilder { self } + /// Write a file to the temporary directory + pub(crate) fn write_file( + self, + relative_path: impl AsRef, + content: &str, + ) -> Result { + let temp_path = SystemPath::from_std_path(self.temp_dir.path()).unwrap(); + let file_path = temp_path.join(relative_path.as_ref()); + + // Create parent directories if they don't exist + if let Some(parent) = file_path.parent() { + std::fs::create_dir_all(parent.as_std_path())?; + } + + std::fs::write(file_path.as_std_path(), content)?; + Ok(self) + } + + /// Write multiple files to the temporary directory + #[expect(dead_code)] + pub(crate) fn write_files( + mut self, + files: impl IntoIterator, + ) -> Result + where + P: AsRef, + C: AsRef, + { + for (path, content) in files { + self = self.write_file(path, content.as_ref())?; + } + Ok(self) + } + /// Build the test server pub(crate) fn build(self) -> Result { - TestServer::new( - self.workspaces, - self.memory_system, - self.client_capabilities, - ) + TestServer::new(self.workspaces, self.temp_dir, self.client_capabilities) } } From c5ad83d319770b0afd9b37dc74332b1d659916ff Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Tue, 22 Jul 2025 13:43:04 +0530 Subject: [PATCH 07/13] Fix tempdir handling --- Cargo.lock | 1 + crates/ty_server/Cargo.toml | 3 +- crates/ty_server/src/server.rs | 38 +++++++++++---- ...ests__publish_diagnostics_on_did_open.snap | 4 +- ...__tests__pull_diagnostics_on_did_open.snap | 2 +- crates/ty_server/src/test.rs | 46 ++++++++++++------- 6 files changed, 65 insertions(+), 29 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dd32a9f400e88e..db673ec2d1fd2d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4295,6 +4295,7 @@ dependencies = [ "libc", "lsp-server", "lsp-types", + "regex", "ruff_db", "ruff_notebook", "ruff_python_ast", diff --git a/crates/ty_server/Cargo.toml b/crates/ty_server/Cargo.toml index 7658dc715b8877..90798704e09f64 100644 --- a/crates/ty_server/Cargo.toml +++ b/crates/ty_server/Cargo.toml @@ -38,7 +38,8 @@ tracing = { workspace = true } tracing-subscriber = { workspace = true, features = ["chrono"] } [dev-dependencies] -insta = { workspace = true, features = ["json"] } +insta = { workspace = true, features = ["filters", "json"] } +regex = { workspace = true } tempfile = { workspace = true } [target.'cfg(target_vendor = "apple")'.dependencies] diff --git a/crates/ty_server/src/server.rs b/crates/ty_server/src/server.rs index fd51abc73049a9..827ec35da5bd55 100644 --- a/crates/ty_server/src/server.rs +++ b/crates/ty_server/src/server.rs @@ -303,20 +303,30 @@ impl Drop for ServerPanicHookHandler { mod tests { use anyhow::Result; use lsp_types::notification::PublishDiagnostics; + use regex::escape; use ruff_db::system::SystemPath; + use tempfile::TempDir; use crate::session::ClientOptions; use crate::test::TestServerBuilder; + fn tempdir_filter(temp_dir: &TempDir) -> String { + format!(r"{}\\?/?", escape(temp_dir.path().to_str().unwrap())) + } + #[test] fn initialization() -> Result<()> { - let test_server = TestServerBuilder::new() + let server = TestServerBuilder::new() .build()? .wait_until_workspaces_are_initialized()?; - let initialization_result = test_server.initialization_result().unwrap(); + let initialization_result = server.initialization_result().unwrap(); + insta::with_settings!({ + filters => vec![(tempdir_filter(server.temp_dir()).as_str(), "[TMP]/")] + }, { insta::assert_json_snapshot!("initialization", initialization_result); + }); Ok(()) } @@ -324,14 +334,18 @@ mod tests { #[test] fn initialization_with_workspace() -> Result<()> { let workspace_root = SystemPath::new("foo"); - let test_server = TestServerBuilder::new() - .with_workspace(workspace_root, ClientOptions::default()) + let server = TestServerBuilder::new() + .with_workspace(workspace_root, ClientOptions::default())? .build()? .wait_until_workspaces_are_initialized()?; - let initialization_result = test_server.initialization_result().unwrap(); + let initialization_result = server.initialization_result().unwrap(); + insta::with_settings!({ + filters => vec![(tempdir_filter(server.temp_dir()).as_str(), "[TMP]/")] + }, { insta::assert_json_snapshot!("initialization_with_workspace", initialization_result); + }); Ok(()) } @@ -346,8 +360,8 @@ def foo() -> str: "; let mut server = TestServerBuilder::new() + .with_workspace(workspace_root, ClientOptions::default())? .write_file(foo, foo_content)? - .with_workspace(workspace_root, ClientOptions::default()) .enable_pull_diagnostics(false) .build()? .wait_until_workspaces_are_initialized()?; @@ -355,7 +369,11 @@ def foo() -> str: server.open_text_document(foo, &foo_content); let diagnostics = server.await_notification::()?; - insta::assert_debug_snapshot!(diagnostics); + insta::with_settings!({ + filters => vec![(tempdir_filter(server.temp_dir()).as_str(), "[TMP]/")] + }, { + insta::assert_debug_snapshot!(diagnostics); + }); Ok(()) } @@ -371,7 +389,7 @@ def foo() -> str: let mut server = TestServerBuilder::new() .write_file(foo, foo_content)? - .with_workspace(workspace_root, ClientOptions::default()) + .with_workspace(workspace_root, ClientOptions::default())? .enable_pull_diagnostics(true) .build()? .wait_until_workspaces_are_initialized()?; @@ -379,7 +397,11 @@ def foo() -> str: server.open_text_document(foo, &foo_content); let diagnostics = server.document_diagnostic_request(foo)?; + insta::with_settings!({ + filters => vec![(tempdir_filter(server.temp_dir()).as_str(), "[TMP]/")] + }, { insta::assert_debug_snapshot!(diagnostics); + }); Ok(()) } diff --git a/crates/ty_server/src/snapshots/ty_server__server__tests__publish_diagnostics_on_did_open.snap b/crates/ty_server/src/snapshots/ty_server__server__tests__publish_diagnostics_on_did_open.snap index e936c4403e7e62..f11d0fbdd43fdb 100644 --- a/crates/ty_server/src/snapshots/ty_server__server__tests__publish_diagnostics_on_did_open.snap +++ b/crates/ty_server/src/snapshots/ty_server__server__tests__publish_diagnostics_on_did_open.snap @@ -10,7 +10,7 @@ PublishDiagnosticsParams { password: None, host: None, port: None, - path: "/src/foo.py", + path: "[TMP]/src/foo.py", query: None, fragment: None, }, @@ -70,7 +70,7 @@ PublishDiagnosticsParams { password: None, host: None, port: None, - path: "/src/foo.py", + path: "[TMP]/src/foo.py", query: None, fragment: None, }, diff --git a/crates/ty_server/src/snapshots/ty_server__server__tests__pull_diagnostics_on_did_open.snap b/crates/ty_server/src/snapshots/ty_server__server__tests__pull_diagnostics_on_did_open.snap index c6fc9120044ff5..3e11eec8b9f041 100644 --- a/crates/ty_server/src/snapshots/ty_server__server__tests__pull_diagnostics_on_did_open.snap +++ b/crates/ty_server/src/snapshots/ty_server__server__tests__pull_diagnostics_on_did_open.snap @@ -64,7 +64,7 @@ Report( password: None, host: None, port: None, - path: "/src/foo.py", + path: "[TMP]/src/foo.py", query: None, fragment: None, }, diff --git a/crates/ty_server/src/test.rs b/crates/ty_server/src/test.rs index 820f897c28de04..7a13055e7afa27 100644 --- a/crates/ty_server/src/test.rs +++ b/crates/ty_server/src/test.rs @@ -7,11 +7,11 @@ use std::collections::hash_map::Entry; use std::collections::{HashMap, VecDeque}; -use std::fmt; use std::num::NonZeroUsize; use std::sync::Arc; use std::thread::JoinHandle; use std::time::Duration; +use std::{fmt, fs}; use anyhow::Result; use crossbeam::channel::RecvTimeoutError; @@ -188,6 +188,10 @@ impl TestServer { Ok(self) } + pub(crate) fn temp_dir(&self) -> &TempDir { + &self.temp_dir + } + /// Wait until the server has initialized all workspaces. /// /// This will wait until the client receives a `workspace/configuration` request from the @@ -514,6 +518,12 @@ impl TestServer { self.initialize_response.as_ref() } + fn file_uri(&self, path: impl AsRef) -> Url { + let temp_dir = SystemPath::from_std_path(self.temp_dir.path()).unwrap(); + Url::from_file_path(temp_dir.join(path.as_ref()).as_std_path()) + .expect("Path must be a valid URL") + } + /// Send a `textDocument/didOpen` notification pub(crate) fn open_text_document( &mut self, @@ -522,7 +532,7 @@ impl TestServer { ) { let params = DidOpenTextDocumentParams { text_document: TextDocumentItem { - uri: Url::from_file_path(path.as_ref()).expect("Path must be a valid URL"), + uri: self.file_uri(path), language_id: "python".to_string(), version: self.next_document_version(), text: content.to_string(), @@ -540,7 +550,7 @@ impl TestServer { ) { let params = DidChangeTextDocumentParams { text_document: VersionedTextDocumentIdentifier { - uri: Url::from_file_path(path.as_ref()).expect("Path must be a valid URL"), + uri: self.file_uri(path), version: self.next_document_version(), }, content_changes: changes, @@ -553,7 +563,7 @@ impl TestServer { pub(crate) fn close_text_document(&mut self, path: impl AsRef) { let params = DidCloseTextDocumentParams { text_document: TextDocumentIdentifier { - uri: Url::from_file_path(path.as_ref()).expect("Path must be a valid URL"), + uri: self.file_uri(path), }, }; self.send_notification::(params); @@ -571,9 +581,10 @@ impl TestServer { &mut self, path: impl AsRef, ) -> Result { - let uri = Url::from_file_path(path.as_ref()).expect("Path must be a valid URL"); let params = DocumentDiagnosticParams { - text_document: TextDocumentIdentifier { uri }, + text_document: TextDocumentIdentifier { + uri: self.file_uri(path), + }, identifier: Some("ty".to_string()), previous_result_id: None, work_done_progress_params: WorkDoneProgressParams::default(), @@ -686,10 +697,12 @@ impl TestServerBuilder { mut self, workspace_root: &SystemPath, options: ClientOptions, - ) -> Self { + ) -> Result { let temp_system_path = SystemPath::from_std_path(self.temp_dir.path()).unwrap(); let workspace_path = temp_system_path.join(workspace_root); + fs::create_dir_all(workspace_path.as_std_path())?; + self.workspaces.push(( WorkspaceFolder { uri: Url::from_file_path(workspace_path.as_std_path()) @@ -698,7 +711,8 @@ impl TestServerBuilder { }, options, )); - self + + Ok(self) } /// Enable or disable pull diagnostics capability @@ -738,33 +752,31 @@ impl TestServerBuilder { /// Write a file to the temporary directory pub(crate) fn write_file( self, - relative_path: impl AsRef, - content: &str, + path: impl AsRef, + content: impl AsRef, ) -> Result { let temp_path = SystemPath::from_std_path(self.temp_dir.path()).unwrap(); - let file_path = temp_path.join(relative_path.as_ref()); + let file_path = temp_path.join(path.as_ref()); // Create parent directories if they don't exist if let Some(parent) = file_path.parent() { std::fs::create_dir_all(parent.as_std_path())?; } - std::fs::write(file_path.as_std_path(), content)?; + fs::write(file_path.as_std_path(), content.as_ref())?; Ok(self) } /// Write multiple files to the temporary directory #[expect(dead_code)] - pub(crate) fn write_files( - mut self, - files: impl IntoIterator, - ) -> Result + pub(crate) fn write_files(mut self, files: I) -> Result where + I: IntoIterator, P: AsRef, C: AsRef, { for (path, content) in files { - self = self.write_file(path, content.as_ref())?; + self = self.write_file(path, content)?; } Ok(self) } From 80cf85058e388b27d7bdc4d982353db15c9ac6b8 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Tue, 22 Jul 2025 13:56:02 +0530 Subject: [PATCH 08/13] Move "initializing logging once" to be done only during tests --- crates/ty_server/src/lib.rs | 2 +- crates/ty_server/src/logging.rs | 90 ++++++++++++++++----------------- crates/ty_server/src/server.rs | 11 ++-- crates/ty_server/src/test.rs | 19 ++++++- 4 files changed, 68 insertions(+), 54 deletions(-) diff --git a/crates/ty_server/src/lib.rs b/crates/ty_server/src/lib.rs index 4e4d03dd340ab4..4496957e0d0e12 100644 --- a/crates/ty_server/src/lib.rs +++ b/crates/ty_server/src/lib.rs @@ -48,7 +48,7 @@ pub fn run_server() -> anyhow::Result<()> { // This is to complement the `LSPSystem` if the document is not available in the index. let fallback_system = Arc::new(OsSystem::new(cwd)); - let server_result = Server::new(worker_threads, connection, fallback_system) + let server_result = Server::new(worker_threads, connection, fallback_system, true) .context("Failed to start server")? .run(); diff --git a/crates/ty_server/src/logging.rs b/crates/ty_server/src/logging.rs index 917b55858edff6..ce92a803ab19ff 100644 --- a/crates/ty_server/src/logging.rs +++ b/crates/ty_server/src/logging.rs @@ -4,7 +4,7 @@ //! are written to `stderr` by default, which should appear in the logs for most LSP clients. A //! `logFile` path can also be specified in the settings, and output will be directed there //! instead. -use std::sync::{Arc, Once}; +use std::sync::Arc; use ruff_db::system::{SystemPath, SystemPathBuf}; use serde::Deserialize; @@ -14,55 +14,51 @@ use tracing_subscriber::fmt::time::ChronoLocal; use tracing_subscriber::fmt::writer::BoxMakeWriter; use tracing_subscriber::layer::SubscriberExt; -static INIT_LOGGING: Once = Once::new(); - pub(crate) fn init_logging(log_level: LogLevel, log_file: Option<&SystemPath>) { - INIT_LOGGING.call_once(|| { - let log_file = log_file - .map(|path| { - // this expands `logFile` so that tildes and environment variables - // are replaced with their values, if possible. - if let Some(expanded) = shellexpand::full(&path.to_string()) - .ok() - .map(|path| SystemPathBuf::from(&*path)) - { - expanded - } else { - path.to_path_buf() - } - }) - .and_then(|path| { - std::fs::OpenOptions::new() - .create(true) - .append(true) - .open(path.as_std_path()) - .map_err(|err| { - #[expect(clippy::print_stderr)] - { - eprintln!("Failed to open file at {path} for logging: {err}"); - } - }) - .ok() - }); + let log_file = log_file + .map(|path| { + // this expands `logFile` so that tildes and environment variables + // are replaced with their values, if possible. + if let Some(expanded) = shellexpand::full(&path.to_string()) + .ok() + .map(|path| SystemPathBuf::from(&*path)) + { + expanded + } else { + path.to_path_buf() + } + }) + .and_then(|path| { + std::fs::OpenOptions::new() + .create(true) + .append(true) + .open(path.as_std_path()) + .map_err(|err| { + #[expect(clippy::print_stderr)] + { + eprintln!("Failed to open file at {path} for logging: {err}"); + } + }) + .ok() + }); - let logger = match log_file { - Some(file) => BoxMakeWriter::new(Arc::new(file)), - None => BoxMakeWriter::new(std::io::stderr), - }; - let is_trace_level = log_level == LogLevel::Trace; - let subscriber = tracing_subscriber::Registry::default().with( - tracing_subscriber::fmt::layer() - .with_timer(ChronoLocal::new("%Y-%m-%d %H:%M:%S.%f".to_string())) - .with_thread_names(is_trace_level) - .with_target(is_trace_level) - .with_ansi(false) - .with_writer(logger) - .with_filter(LogLevelFilter { filter: log_level }), - ); + let logger = match log_file { + Some(file) => BoxMakeWriter::new(Arc::new(file)), + None => BoxMakeWriter::new(std::io::stderr), + }; + let is_trace_level = log_level == LogLevel::Trace; + let subscriber = tracing_subscriber::Registry::default().with( + tracing_subscriber::fmt::layer() + .with_timer(ChronoLocal::new("%Y-%m-%d %H:%M:%S.%f".to_string())) + .with_thread_names(is_trace_level) + .with_target(is_trace_level) + .with_ansi(false) + .with_writer(logger) + .with_filter(LogLevelFilter { filter: log_level }), + ); - tracing::subscriber::set_global_default(subscriber) - .expect("should be able to set global default subscriber"); - }); + tracing::subscriber::set_global_default(subscriber) + .expect("should be able to set global default subscriber"); } /// The log level for the server as provided by the client during initialization. diff --git a/crates/ty_server/src/server.rs b/crates/ty_server/src/server.rs index 827ec35da5bd55..e9e37b4e066382 100644 --- a/crates/ty_server/src/server.rs +++ b/crates/ty_server/src/server.rs @@ -40,6 +40,7 @@ impl Server { worker_threads: NonZeroUsize, connection: Connection, native_system: Arc, + initialize_logging: bool, ) -> crate::Result { let (id, init_value) = connection.initialize_start()?; let init_params: InitializeParams = serde_json::from_value(init_value)?; @@ -76,10 +77,12 @@ impl Server { let (main_loop_sender, main_loop_receiver) = crossbeam::channel::bounded(32); let client = Client::new(main_loop_sender.clone(), connection.sender.clone()); - crate::logging::init_logging( - global_options.tracing.log_level.unwrap_or_default(), - global_options.tracing.log_file.as_deref(), - ); + if initialize_logging { + crate::logging::init_logging( + global_options.tracing.log_level.unwrap_or_default(), + global_options.tracing.log_file.as_deref(), + ); + } tracing::debug!("Version: {version}"); diff --git a/crates/ty_server/src/test.rs b/crates/ty_server/src/test.rs index 7a13055e7afa27..40ad111160a069 100644 --- a/crates/ty_server/src/test.rs +++ b/crates/ty_server/src/test.rs @@ -8,7 +8,7 @@ use std::collections::hash_map::Entry; use std::collections::{HashMap, VecDeque}; use std::num::NonZeroUsize; -use std::sync::Arc; +use std::sync::{Arc, OnceLock}; use std::thread::JoinHandle; use std::time::Duration; use std::{fmt, fs}; @@ -37,12 +37,25 @@ use ruff_db::system::{OsSystem, SystemPath, TestSystem}; use serde::de::DeserializeOwned; use tempfile::TempDir; +use crate::logging::{LogLevel, init_logging}; use crate::server::Server; use crate::session::ClientOptions; /// Number of times to retry receiving a message before giving up const RETRY_COUNT: usize = 5; +static INIT_TRACING: OnceLock<()> = OnceLock::new(); + +/// Setup tracing for the test server. +/// +/// This will make sure that the tracing subscriber is initialized only once, so that running +/// multiple tests does not cause multiple subscribers to be registered. +fn setup_tracing() { + INIT_TRACING.get_or_init(|| { + init_logging(LogLevel::Info, None); + }); +} + /// Errors that can occur during testing #[derive(thiserror::Error, Debug)] pub(crate) enum TestServerError { @@ -116,6 +129,8 @@ impl TestServer { temp_dir: TempDir, capabilities: ClientCapabilities, ) -> Result { + setup_tracing(); + let (server_connection, client_connection) = Connection::memory(); // Create OS system with the temp directory as cwd @@ -128,7 +143,7 @@ impl TestServer { let worker_threads = NonZeroUsize::new(1).unwrap(); let test_system = Arc::new(TestSystem::new(os_system)); - match Server::new(worker_threads, server_connection, test_system) { + match Server::new(worker_threads, server_connection, test_system, false) { Ok(server) => { if let Err(err) = server.run() { panic!("Server stopped with error: {err:?}"); From ce493beb2f84ec5be4ebe54c43a09ed3bd128d71 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Tue, 22 Jul 2025 15:27:55 +0530 Subject: [PATCH 09/13] Use temp dir implementation from cli tests --- Cargo.lock | 1 + crates/ty_server/Cargo.toml | 1 + crates/ty_server/src/server.rs | 36 +---- ...ests__publish_diagnostics_on_did_open.snap | 4 +- ...__tests__pull_diagnostics_on_did_open.snap | 2 +- crates/ty_server/src/test.rs | 131 +++++++++++++----- 6 files changed, 107 insertions(+), 68 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index db673ec2d1fd2d..24c3acd70cfb9c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4290,6 +4290,7 @@ dependencies = [ "anyhow", "bitflags 2.9.1", "crossbeam", + "dunce", "insta", "jod-thread", "libc", diff --git a/crates/ty_server/Cargo.toml b/crates/ty_server/Cargo.toml index 90798704e09f64..f0efbfe1c50b29 100644 --- a/crates/ty_server/Cargo.toml +++ b/crates/ty_server/Cargo.toml @@ -38,6 +38,7 @@ tracing = { workspace = true } tracing-subscriber = { workspace = true, features = ["chrono"] } [dev-dependencies] +dunce = { workspace = true } insta = { workspace = true, features = ["filters", "json"] } regex = { workspace = true } tempfile = { workspace = true } diff --git a/crates/ty_server/src/server.rs b/crates/ty_server/src/server.rs index e9e37b4e066382..4287b6ed2e7499 100644 --- a/crates/ty_server/src/server.rs +++ b/crates/ty_server/src/server.rs @@ -306,30 +306,20 @@ impl Drop for ServerPanicHookHandler { mod tests { use anyhow::Result; use lsp_types::notification::PublishDiagnostics; - use regex::escape; use ruff_db::system::SystemPath; - use tempfile::TempDir; use crate::session::ClientOptions; use crate::test::TestServerBuilder; - fn tempdir_filter(temp_dir: &TempDir) -> String { - format!(r"{}\\?/?", escape(temp_dir.path().to_str().unwrap())) - } - #[test] fn initialization() -> Result<()> { - let server = TestServerBuilder::new() + let server = TestServerBuilder::new()? .build()? .wait_until_workspaces_are_initialized()?; let initialization_result = server.initialization_result().unwrap(); - insta::with_settings!({ - filters => vec![(tempdir_filter(server.temp_dir()).as_str(), "[TMP]/")] - }, { insta::assert_json_snapshot!("initialization", initialization_result); - }); Ok(()) } @@ -337,18 +327,14 @@ mod tests { #[test] fn initialization_with_workspace() -> Result<()> { let workspace_root = SystemPath::new("foo"); - let server = TestServerBuilder::new() + let server = TestServerBuilder::new()? .with_workspace(workspace_root, ClientOptions::default())? .build()? .wait_until_workspaces_are_initialized()?; let initialization_result = server.initialization_result().unwrap(); - insta::with_settings!({ - filters => vec![(tempdir_filter(server.temp_dir()).as_str(), "[TMP]/")] - }, { insta::assert_json_snapshot!("initialization_with_workspace", initialization_result); - }); Ok(()) } @@ -362,9 +348,9 @@ def foo() -> str: return 42 "; - let mut server = TestServerBuilder::new() + let mut server = TestServerBuilder::new()? .with_workspace(workspace_root, ClientOptions::default())? - .write_file(foo, foo_content)? + .with_file(foo, foo_content)? .enable_pull_diagnostics(false) .build()? .wait_until_workspaces_are_initialized()?; @@ -372,11 +358,7 @@ def foo() -> str: server.open_text_document(foo, &foo_content); let diagnostics = server.await_notification::()?; - insta::with_settings!({ - filters => vec![(tempdir_filter(server.temp_dir()).as_str(), "[TMP]/")] - }, { - insta::assert_debug_snapshot!(diagnostics); - }); + insta::assert_debug_snapshot!(diagnostics); Ok(()) } @@ -390,9 +372,9 @@ def foo() -> str: return 42 "; - let mut server = TestServerBuilder::new() - .write_file(foo, foo_content)? + let mut server = TestServerBuilder::new()? .with_workspace(workspace_root, ClientOptions::default())? + .with_file(foo, foo_content)? .enable_pull_diagnostics(true) .build()? .wait_until_workspaces_are_initialized()?; @@ -400,11 +382,7 @@ def foo() -> str: server.open_text_document(foo, &foo_content); let diagnostics = server.document_diagnostic_request(foo)?; - insta::with_settings!({ - filters => vec![(tempdir_filter(server.temp_dir()).as_str(), "[TMP]/")] - }, { insta::assert_debug_snapshot!(diagnostics); - }); Ok(()) } diff --git a/crates/ty_server/src/snapshots/ty_server__server__tests__publish_diagnostics_on_did_open.snap b/crates/ty_server/src/snapshots/ty_server__server__tests__publish_diagnostics_on_did_open.snap index f11d0fbdd43fdb..5ed7b5a20be536 100644 --- a/crates/ty_server/src/snapshots/ty_server__server__tests__publish_diagnostics_on_did_open.snap +++ b/crates/ty_server/src/snapshots/ty_server__server__tests__publish_diagnostics_on_did_open.snap @@ -10,7 +10,7 @@ PublishDiagnosticsParams { password: None, host: None, port: None, - path: "[TMP]/src/foo.py", + path: "/src/foo.py", query: None, fragment: None, }, @@ -70,7 +70,7 @@ PublishDiagnosticsParams { password: None, host: None, port: None, - path: "[TMP]/src/foo.py", + path: "/src/foo.py", query: None, fragment: None, }, diff --git a/crates/ty_server/src/snapshots/ty_server__server__tests__pull_diagnostics_on_did_open.snap b/crates/ty_server/src/snapshots/ty_server__server__tests__pull_diagnostics_on_did_open.snap index 3e11eec8b9f041..27f94d7b4d396b 100644 --- a/crates/ty_server/src/snapshots/ty_server__server__tests__pull_diagnostics_on_did_open.snap +++ b/crates/ty_server/src/snapshots/ty_server__server__tests__pull_diagnostics_on_did_open.snap @@ -64,7 +64,7 @@ Report( password: None, host: None, port: None, - path: "[TMP]/src/foo.py", + path: "/src/foo.py", query: None, fragment: None, }, diff --git a/crates/ty_server/src/test.rs b/crates/ty_server/src/test.rs index 40ad111160a069..9923f78dcaf4ba 100644 --- a/crates/ty_server/src/test.rs +++ b/crates/ty_server/src/test.rs @@ -13,8 +13,9 @@ use std::thread::JoinHandle; use std::time::Duration; use std::{fmt, fs}; -use anyhow::Result; +use anyhow::{Context, Result}; use crossbeam::channel::RecvTimeoutError; +use insta::internals::SettingsBindDropGuard; use lsp_server::{Connection, Message, RequestId, Response, ResponseError}; use lsp_types::notification::{ DidChangeTextDocument, DidChangeWatchedFiles, DidCloseTextDocument, DidOpenTextDocument, Exit, @@ -33,7 +34,7 @@ use lsp_types::{ TextDocumentItem, Url, VersionedTextDocumentIdentifier, WorkDoneProgressParams, WorkspaceClientCapabilities, WorkspaceFolder, }; -use ruff_db::system::{OsSystem, SystemPath, TestSystem}; +use ruff_db::system::{OsSystem, SystemPath, SystemPathBuf, TestSystem}; use serde::de::DeserializeOwned; use tempfile::TempDir; @@ -92,10 +93,10 @@ pub(crate) struct TestServer { /// the connection to be cleaned up properly. client_connection: Option, - /// Temporary directory that holds all test files. + /// Test directory that holds all test files. /// /// This directory is automatically cleaned up when the [`TestServer`] is dropped. - temp_dir: TempDir, + test_dir: TestDir, /// Incrementing counter to automatically generate request IDs request_counter: i32, @@ -124,18 +125,17 @@ pub(crate) struct TestServer { impl TestServer { /// Create a new test server with the given workspace configurations - pub(crate) fn new( + fn new( workspaces: Vec<(WorkspaceFolder, ClientOptions)>, - temp_dir: TempDir, + test_dir: TestDir, capabilities: ClientCapabilities, ) -> Result { setup_tracing(); let (server_connection, client_connection) = Connection::memory(); - // Create OS system with the temp directory as cwd - let temp_path = SystemPath::from_std_path(temp_dir.path()).unwrap(); - let os_system = OsSystem::new(temp_path); + // Create OS system with the test directory as cwd + let os_system = OsSystem::new(test_dir.root()); // Start the server in a separate thread let server_thread = std::thread::spawn(move || { @@ -168,7 +168,7 @@ impl TestServer { Self { server_thread: Some(server_thread), client_connection: Some(client_connection), - temp_dir, + test_dir, request_counter: 0, version_counter: 0, responses: HashMap::new(), @@ -203,10 +203,6 @@ impl TestServer { Ok(self) } - pub(crate) fn temp_dir(&self) -> &TempDir { - &self.temp_dir - } - /// Wait until the server has initialized all workspaces. /// /// This will wait until the client receives a `workspace/configuration` request from the @@ -534,8 +530,7 @@ impl TestServer { } fn file_uri(&self, path: impl AsRef) -> Url { - let temp_dir = SystemPath::from_std_path(self.temp_dir.path()).unwrap(); - Url::from_file_path(temp_dir.join(path.as_ref()).as_std_path()) + Url::from_file_path(self.test_dir.root().join(path.as_ref()).as_std_path()) .expect("Path must be a valid URL") } @@ -613,7 +608,7 @@ impl TestServer { impl fmt::Debug for TestServer { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("TestServer") - .field("temp_dir", &self.temp_dir.path()) + .field("temp_dir", &self.test_dir.root()) .field("request_counter", &self.request_counter) .field("version_counter", &self.version_counter) .field("responses", &self.responses) @@ -675,14 +670,14 @@ impl Drop for TestServer { /// Builder for creating test servers with specific configurations pub(crate) struct TestServerBuilder { - temp_dir: TempDir, + test_dir: TestDir, workspaces: Vec<(WorkspaceFolder, ClientOptions)>, client_capabilities: ClientCapabilities, } impl TestServerBuilder { /// Create a new builder - pub(crate) fn new() -> Self { + pub(crate) fn new() -> Result { // Default client capabilities for the test server. These are assumptions made by the real // server and are common for most clients: // @@ -700,22 +695,28 @@ impl TestServerBuilder { ..Default::default() }; - Self { + Ok(Self { workspaces: Vec::new(), - temp_dir: TempDir::new().expect("should be able to create temporary directory"), + test_dir: TestDir::new()?, client_capabilities, - } + }) } - /// Add a workspace configuration + /// Add a workspace to the test server with the given root path and options. + /// + /// This option will be used to respond to the `workspace/configuration` request that the + /// server will send to the client. pub(crate) fn with_workspace( mut self, workspace_root: &SystemPath, options: ClientOptions, ) -> Result { - let temp_system_path = SystemPath::from_std_path(self.temp_dir.path()).unwrap(); - let workspace_path = temp_system_path.join(workspace_root); + // TODO: Support multiple workspaces in the test server + if self.workspaces.len() == 1 { + anyhow::bail!("Test server doesn't support multiple workspaces yet"); + } + let workspace_path = self.test_dir.root().join(workspace_root); fs::create_dir_all(workspace_path.as_std_path())?; self.workspaces.push(( @@ -764,40 +765,98 @@ impl TestServerBuilder { self } - /// Write a file to the temporary directory - pub(crate) fn write_file( + /// Write a file to the test directory + pub(crate) fn with_file( self, path: impl AsRef, content: impl AsRef, ) -> Result { - let temp_path = SystemPath::from_std_path(self.temp_dir.path()).unwrap(); - let file_path = temp_path.join(path.as_ref()); - - // Create parent directories if they don't exist + let file_path = self.test_dir.root().join(path.as_ref()); + // Ensure parent directories exists if let Some(parent) = file_path.parent() { - std::fs::create_dir_all(parent.as_std_path())?; + fs::create_dir_all(parent.as_std_path())?; } - fs::write(file_path.as_std_path(), content.as_ref())?; Ok(self) } /// Write multiple files to the temporary directory #[expect(dead_code)] - pub(crate) fn write_files(mut self, files: I) -> Result + pub(crate) fn with_files(mut self, files: I) -> Result where I: IntoIterator, P: AsRef, C: AsRef, { for (path, content) in files { - self = self.write_file(path, content)?; + self = self.with_file(path, content)?; } Ok(self) } /// Build the test server pub(crate) fn build(self) -> Result { - TestServer::new(self.workspaces, self.temp_dir, self.client_capabilities) + TestServer::new(self.workspaces, self.test_dir, self.client_capabilities) + } +} + +/// A temporary directory for testing purposes. +/// +/// This holds the insta settings scope that filters out the temporary directory path from +/// snapshots. +/// +/// This is similar to the `CliTest` in `ty` crate. +struct TestDir { + _temp_dir: TempDir, + _settings_scope: SettingsBindDropGuard, + project_dir: SystemPathBuf, +} + +impl TestDir { + pub(crate) fn new() -> anyhow::Result { + let temp_dir = TempDir::new()?; + + // Canonicalize the tempdir path because macos uses symlinks for tempdirs + // and that doesn't play well with our snapshot filtering. + // Simplify with dunce because otherwise we get UNC paths on Windows. + let project_dir = SystemPathBuf::from_path_buf( + dunce::simplified( + &temp_dir + .path() + .canonicalize() + .context("Failed to canonicalize project path")?, + ) + .to_path_buf(), + ) + .map_err(|path| { + anyhow::anyhow!( + "Failed to create test directory: `{}` contains non-Unicode characters", + path.display() + ) + })?; + + let mut settings = insta::Settings::clone_current(); + settings.add_filter(&tempdir_filter(&project_dir), "/"); + settings.add_filter(r#"\\(\w\w|\s|\.|")"#, "/$1"); + settings.add_filter( + r#"The system cannot find the file specified."#, + "No such file or directory", + ); + + let settings_scope = settings.bind_to_scope(); + + Ok(Self { + project_dir, + _temp_dir: temp_dir, + _settings_scope: settings_scope, + }) + } + + pub(crate) fn root(&self) -> &SystemPath { + &self.project_dir } } + +fn tempdir_filter(path: &SystemPath) -> String { + format!(r"{}\\?/?", regex::escape(path.as_str())) +} From 20b61af0a1d30ee5a28fdaaf875f300406262c87 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Wed, 23 Jul 2025 11:53:04 +0530 Subject: [PATCH 10/13] Address review comments --- crates/ty_server/src/server.rs | 4 +- crates/ty_server/src/test.rs | 162 ++++++++++++++++++++------------- 2 files changed, 99 insertions(+), 67 deletions(-) diff --git a/crates/ty_server/src/server.rs b/crates/ty_server/src/server.rs index 4287b6ed2e7499..df01a0a8b3f922 100644 --- a/crates/ty_server/src/server.rs +++ b/crates/ty_server/src/server.rs @@ -355,7 +355,7 @@ def foo() -> str: .build()? .wait_until_workspaces_are_initialized()?; - server.open_text_document(foo, &foo_content); + server.open_text_document(foo, &foo_content, 1); let diagnostics = server.await_notification::()?; insta::assert_debug_snapshot!(diagnostics); @@ -379,7 +379,7 @@ def foo() -> str: .build()? .wait_until_workspaces_are_initialized()?; - server.open_text_document(foo, &foo_content); + server.open_text_document(foo, &foo_content, 1); let diagnostics = server.document_diagnostic_request(foo)?; insta::assert_debug_snapshot!(diagnostics); diff --git a/crates/ty_server/src/test.rs b/crates/ty_server/src/test.rs index 9923f78dcaf4ba..42976bd8cb2687 100644 --- a/crates/ty_server/src/test.rs +++ b/crates/ty_server/src/test.rs @@ -13,7 +13,7 @@ use std::thread::JoinHandle; use std::time::Duration; use std::{fmt, fs}; -use anyhow::{Context, Result}; +use anyhow::{Context, Result, anyhow}; use crossbeam::channel::RecvTimeoutError; use insta::internals::SettingsBindDropGuard; use lsp_server::{Connection, Message, RequestId, Response, ResponseError}; @@ -53,7 +53,7 @@ static INIT_TRACING: OnceLock<()> = OnceLock::new(); /// multiple tests does not cause multiple subscribers to be registered. fn setup_tracing() { INIT_TRACING.get_or_init(|| { - init_logging(LogLevel::Info, None); + init_logging(LogLevel::Debug, None); }); } @@ -70,8 +70,17 @@ pub(crate) enum TestServerError { #[error("Got a duplicate response for request ID {0}: {1:?}")] DuplicateResponse(RequestId, Response), - #[error("Timeout while waiting for a message from the server")] - RecvTimeoutError, + #[error("Failed to receive message from server: {0}")] + RecvTimeoutError(RecvTimeoutError), +} + +impl TestServerError { + fn is_disconnected(&self) -> bool { + matches!( + self, + TestServerError::RecvTimeoutError(RecvTimeoutError::Disconnected) + ) + } } /// A test server for the ty language server that provides helpers for sending requests, @@ -96,14 +105,11 @@ pub(crate) struct TestServer { /// Test directory that holds all test files. /// /// This directory is automatically cleaned up when the [`TestServer`] is dropped. - test_dir: TestDir, + test_dir: TestContext, /// Incrementing counter to automatically generate request IDs request_counter: i32, - /// Simple incrementing document version counter - version_counter: i32, - /// A mapping of request IDs to responses received from the server responses: HashMap, @@ -127,7 +133,7 @@ impl TestServer { /// Create a new test server with the given workspace configurations fn new( workspaces: Vec<(WorkspaceFolder, ClientOptions)>, - test_dir: TestDir, + test_dir: TestContext, capabilities: ClientCapabilities, ) -> Result { setup_tracing(); @@ -170,7 +176,6 @@ impl TestServer { client_connection: Some(client_connection), test_dir, request_counter: 0, - version_counter: 0, responses: HashMap::new(), notifications: VecDeque::new(), requests: VecDeque::new(), @@ -222,11 +227,13 @@ impl TestServer { // implementation which happens everytime the test ends. match self.receive(Some(Duration::from_millis(10))) { Ok(()) => {} - Err(TestServerError::RecvTimeoutError) => { + Err(TestServerError::RecvTimeoutError(_)) => { // Only break if we have no more messages to process. break; } - Err(_) => {} + Err(err) => { + tracing::error!("Error while draining messages: {err:?}"); + } } } } @@ -267,12 +274,6 @@ impl TestServer { RequestId::from(self.request_counter) } - /// Generate a new document version - fn next_document_version(&mut self) -> i32 { - self.version_counter += 1; - self.version_counter - } - /// Send a message to the server. /// /// # Panics @@ -328,8 +329,6 @@ impl TestServer { /// [`send_request`]: TestServer::send_request pub(crate) fn await_response(&mut self, id: RequestId) -> Result { loop { - self.receive(None)?; - if let Some(response) = self.responses.remove(&id) { match response { Response { @@ -351,6 +350,8 @@ impl TestServer { } } } + + self.receive_or_panic()?; } } @@ -364,8 +365,10 @@ impl TestServer { /// This method will remove the notification from the internal data structure, so it should /// only be called if the notification is expected to be sent by the server. pub(crate) fn await_notification(&mut self) -> Result { - for _ in 0..RETRY_COUNT { - self.receive(None)?; + for retry_count in 0..RETRY_COUNT { + if retry_count > 0 { + tracing::info!("Retrying to receive `{}` notification", N::METHOD); + } let notification = self .notifications .iter() @@ -374,7 +377,7 @@ impl TestServer { if let Some(notification) = notification { return Ok(serde_json::from_value(notification.params)?); } - tracing::info!("Retrying to receive `{}` notification", N::METHOD); + self.receive_or_panic()?; } Err(anyhow::anyhow!( "Failed to receive `{}` notification after {RETRY_COUNT} retries", @@ -393,8 +396,10 @@ impl TestServer { /// This method will remove the request from the internal data structure, so it should only be /// called if the request is expected to be sent by the server. pub(crate) fn await_request(&mut self) -> Result<(RequestId, R::Params)> { - for _ in 0..RETRY_COUNT { - self.receive(None)?; + for retry_count in 0..RETRY_COUNT { + if retry_count > 0 { + tracing::info!("Retrying to receive `{}` request", R::METHOD); + } let request = self .requests .iter() @@ -404,7 +409,7 @@ impl TestServer { let params = serde_json::from_value(request.params)?; return Ok((request.id, params)); } - tracing::info!("Retrying to receive `{}` request", R::METHOD); + self.receive_or_panic()?; } Err(anyhow::anyhow!( "Failed to receive `{}` request after {RETRY_COUNT} retries", @@ -422,19 +427,37 @@ impl TestServer { fn receive(&mut self, timeout: Option) -> Result<(), TestServerError> { static DEFAULT_TIMEOUT: Duration = Duration::from_secs(1); - match self + let message = self .client_connection .as_ref() .unwrap() .receiver .recv_timeout(timeout.unwrap_or(DEFAULT_TIMEOUT)) - { - Ok(message) => self.handle_message(message), - Err(RecvTimeoutError::Timeout) => Err(TestServerError::RecvTimeoutError), - Err(RecvTimeoutError::Disconnected) => { + .map_err(TestServerError::RecvTimeoutError)?; + + self.handle_message(message)?; + + // for message in self.client_connection.as_ref().unwrap().receiver.try_iter() { + // self.handle_message(message)?; + // } + + Ok(()) + } + + /// This is a convenience method that's same as [`receive`], but panics if the server got + /// disconnected. It will pass other errors as is. + /// + /// [`receive`]: TestServer::receive + #[allow(clippy::result_large_err)] + fn receive_or_panic(&mut self) -> Result<(), TestServerError> { + if let Err(err) = self.receive(None) { + if err.is_disconnected() { self.panic_on_server_disconnect(); + } else { + return Err(err); } } + Ok(()) } /// Handle the incoming message from the server. @@ -539,12 +562,13 @@ impl TestServer { &mut self, path: impl AsRef, content: &impl ToString, + version: i32, ) { let params = DidOpenTextDocumentParams { text_document: TextDocumentItem { uri: self.file_uri(path), language_id: "python".to_string(), - version: self.next_document_version(), + version, text: content.to_string(), }, }; @@ -557,11 +581,12 @@ impl TestServer { &mut self, path: impl AsRef, changes: Vec, + version: i32, ) { let params = DidChangeTextDocumentParams { text_document: VersionedTextDocumentIdentifier { uri: self.file_uri(path), - version: self.next_document_version(), + version, }, content_changes: changes, }; @@ -610,7 +635,6 @@ impl fmt::Debug for TestServer { f.debug_struct("TestServer") .field("temp_dir", &self.test_dir.root()) .field("request_counter", &self.request_counter) - .field("version_counter", &self.version_counter) .field("responses", &self.responses) .field("notifications", &self.notifications) .field("server_requests", &self.requests) @@ -629,37 +653,35 @@ impl Drop for TestServer { // // The `server_thread` could be `None` if the server exited unexpectedly or panicked or if // it dropped the client connection. - let shutdown_error = self - .server_thread - .is_some() - .then(|| { - let shutdown_id = self.send_request::(()); - match self.await_response::<()>(shutdown_id) { - Ok(()) => { - self.send_notification::(()); - None - } - Err(err) => Some(format!("Failed to get shutdown response: {err:?}")), + let shutdown_error = if self.server_thread.is_some() { + let shutdown_id = self.send_request::(()); + match self.await_response::<()>(shutdown_id) { + Ok(()) => { + self.send_notification::(()); + None } - }) - .flatten(); + Err(err) => Some(format!("Failed to get shutdown response: {err:?}")), + } + } else { + None + }; if let Some(_client_connection) = self.client_connection.take() { // Drop the client connection before joining the server thread to avoid any hangs // in case the server didn't respond to the shutdown request. } + if std::thread::panicking() { + // If the test server panicked, avoid further assertions. + return; + } + if let Some(server_thread) = self.server_thread.take() { if let Err(err) = server_thread.join() { panic!("Panic in the server thread: {err:?}"); } } - if std::thread::panicking() { - // If the test server panicked, avoid further assertions. - return; - } - if let Some(error) = shutdown_error { panic!("Test server did not shut down gracefully: {error}"); } @@ -670,7 +692,7 @@ impl Drop for TestServer { /// Builder for creating test servers with specific configurations pub(crate) struct TestServerBuilder { - test_dir: TestDir, + test_dir: TestContext, workspaces: Vec<(WorkspaceFolder, ClientOptions)>, client_capabilities: ClientCapabilities, } @@ -697,7 +719,7 @@ impl TestServerBuilder { Ok(Self { workspaces: Vec::new(), - test_dir: TestDir::new()?, + test_dir: TestContext::new()?, client_capabilities, }) } @@ -721,8 +743,9 @@ impl TestServerBuilder { self.workspaces.push(( WorkspaceFolder { - uri: Url::from_file_path(workspace_path.as_std_path()) - .expect("workspace root should be a valid URL"), + uri: Url::from_file_path(workspace_path.as_std_path()).map_err(|()| { + anyhow!("Failed to convert workspace path to URL: {workspace_path}") + })?, name: workspace_root.file_name().unwrap_or("test").to_string(), }, options, @@ -800,19 +823,20 @@ impl TestServerBuilder { } } -/// A temporary directory for testing purposes. +/// A context specific to a server test. /// -/// This holds the insta settings scope that filters out the temporary directory path from -/// snapshots. +/// This creates a temporary directory that is used as the current working directory for the server +/// in which the test files are stored. This also holds the insta settings scope that filters out +/// the temporary directory path from snapshots. /// /// This is similar to the `CliTest` in `ty` crate. -struct TestDir { +struct TestContext { _temp_dir: TempDir, _settings_scope: SettingsBindDropGuard, project_dir: SystemPathBuf, } -impl TestDir { +impl TestContext { pub(crate) fn new() -> anyhow::Result { let temp_dir = TempDir::new()?; @@ -829,14 +853,22 @@ impl TestDir { .to_path_buf(), ) .map_err(|path| { - anyhow::anyhow!( + anyhow!( "Failed to create test directory: `{}` contains non-Unicode characters", path.display() ) })?; let mut settings = insta::Settings::clone_current(); - settings.add_filter(&tempdir_filter(&project_dir), "/"); + settings.add_filter(&tempdir_filter(project_dir.as_str()), "/"); + settings.add_filter( + &tempdir_filter( + Url::from_file_path(project_dir.as_std_path()) + .map_err(|()| anyhow!("Failed to convert root directory to url"))? + .path(), + ), + "//", + ); settings.add_filter(r#"\\(\w\w|\s|\.|")"#, "/$1"); settings.add_filter( r#"The system cannot find the file specified."#, @@ -857,6 +889,6 @@ impl TestDir { } } -fn tempdir_filter(path: &SystemPath) -> String { - format!(r"{}\\?/?", regex::escape(path.as_str())) +fn tempdir_filter(path: impl AsRef) -> String { + format!(r"{}\\?/?", regex::escape(path.as_ref())) } From 22d5ad213095f783f32b606d2f880ed5bba99929 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Wed, 23 Jul 2025 12:13:46 +0530 Subject: [PATCH 11/13] Box the Response to reduce error size --- crates/ty_server/src/test.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/crates/ty_server/src/test.rs b/crates/ty_server/src/test.rs index 42976bd8cb2687..fa3783496e663a 100644 --- a/crates/ty_server/src/test.rs +++ b/crates/ty_server/src/test.rs @@ -65,10 +65,10 @@ pub(crate) enum TestServerError { ResponseError(ResponseError), #[error("Invalid response message for request {0}: {1:?}")] - InvalidResponse(RequestId, Response), + InvalidResponse(RequestId, Box), #[error("Got a duplicate response for request ID {0}: {1:?}")] - DuplicateResponse(RequestId, Response), + DuplicateResponse(RequestId, Box), #[error("Failed to receive message from server: {0}")] RecvTimeoutError(RecvTimeoutError), @@ -346,7 +346,7 @@ impl TestServer { return Err(TestServerError::ResponseError(err).into()); } response => { - return Err(TestServerError::InvalidResponse(id, response).into()); + return Err(TestServerError::InvalidResponse(id, Box::new(response)).into()); } } } @@ -423,7 +423,6 @@ impl TestServer { /// within that time, it will return an error. /// /// If `timeout` is `None`, it will use a default timeout of 1 second. - #[allow(clippy::result_large_err)] fn receive(&mut self, timeout: Option) -> Result<(), TestServerError> { static DEFAULT_TIMEOUT: Duration = Duration::from_secs(1); @@ -448,7 +447,6 @@ impl TestServer { /// disconnected. It will pass other errors as is. /// /// [`receive`]: TestServer::receive - #[allow(clippy::result_large_err)] fn receive_or_panic(&mut self) -> Result<(), TestServerError> { if let Err(err) = self.receive(None) { if err.is_disconnected() { @@ -466,7 +464,6 @@ impl TestServer { /// - Requests are stored in `self.requests` /// - Responses are stored in `self.responses` with the request ID as the key /// - Notifications are stored in `self.notifications` - #[allow(clippy::result_large_err)] fn handle_message(&mut self, message: Message) -> Result<(), TestServerError> { match message { Message::Request(request) => { @@ -476,7 +473,7 @@ impl TestServer { Entry::Occupied(existing) => { return Err(TestServerError::DuplicateResponse( response.id, - existing.get().clone(), + Box::new(existing.get().clone()), )); } Entry::Vacant(entry) => { From bce74ca2ff194d5b41470d8fb207a58a82a9d0aa Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Wed, 23 Jul 2025 12:14:46 +0530 Subject: [PATCH 12/13] Avoid leading slash in temp dir replacement --- crates/ty_server/src/test.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ty_server/src/test.rs b/crates/ty_server/src/test.rs index fa3783496e663a..5bae1f6536f4da 100644 --- a/crates/ty_server/src/test.rs +++ b/crates/ty_server/src/test.rs @@ -864,7 +864,7 @@ impl TestContext { .map_err(|()| anyhow!("Failed to convert root directory to url"))? .path(), ), - "//", + "/", ); settings.add_filter(r#"\\(\w\w|\s|\.|")"#, "/$1"); settings.add_filter( From 1940ea23f8ebe7b32bf775dd620028417df3e841 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Wed, 23 Jul 2025 12:19:09 +0530 Subject: [PATCH 13/13] Collect all messages during receive --- crates/ty_server/src/test.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/crates/ty_server/src/test.rs b/crates/ty_server/src/test.rs index 5bae1f6536f4da..022a838cb65d35 100644 --- a/crates/ty_server/src/test.rs +++ b/crates/ty_server/src/test.rs @@ -35,6 +35,7 @@ use lsp_types::{ WorkspaceClientCapabilities, WorkspaceFolder, }; use ruff_db::system::{OsSystem, SystemPath, SystemPathBuf, TestSystem}; +use rustc_hash::FxHashMap; use serde::de::DeserializeOwned; use tempfile::TempDir; @@ -111,7 +112,7 @@ pub(crate) struct TestServer { request_counter: i32, /// A mapping of request IDs to responses received from the server - responses: HashMap, + responses: FxHashMap, /// An ordered queue of all the notifications received from the server notifications: VecDeque, @@ -176,7 +177,7 @@ impl TestServer { client_connection: Some(client_connection), test_dir, request_counter: 0, - responses: HashMap::new(), + responses: FxHashMap::default(), notifications: VecDeque::new(), requests: VecDeque::new(), initialize_response: None, @@ -426,19 +427,16 @@ impl TestServer { fn receive(&mut self, timeout: Option) -> Result<(), TestServerError> { static DEFAULT_TIMEOUT: Duration = Duration::from_secs(1); - let message = self - .client_connection - .as_ref() - .unwrap() - .receiver + let receiver = self.client_connection.as_ref().unwrap().receiver.clone(); + let message = receiver .recv_timeout(timeout.unwrap_or(DEFAULT_TIMEOUT)) .map_err(TestServerError::RecvTimeoutError)?; self.handle_message(message)?; - // for message in self.client_connection.as_ref().unwrap().receiver.try_iter() { - // self.handle_message(message)?; - // } + for message in receiver.try_iter() { + self.handle_message(message)?; + } Ok(()) }