Skip to content

[ty] Move server tests as integration tests #19522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dhruvmanila merged 3 commits into from
Jul 24, 2025
Merged

[ty] Move server tests as integration tests #19522

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dbc9166
[ty] Move server tests as integration tests
dhruvmanila Jul 24, 2025
32c3cbe
Move the test setup folders as well
dhruvmanila Jul 24, 2025
acb539b
Address review comments
dhruvmanila Jul 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions crates/ty_server/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,15 @@ thiserror = { workspace = true }
tracing = { workspace = true }
tracing-subscriber = { workspace = true, features = ["chrono"] }

dunce = { workspace = true, optional = true }
insta = { workspace = true, features = ["filters", "json"], optional = true }
regex = { workspace = true , optional = true }
tempfile = { workspace = true , optional = true }

[features]
testing = ["dunce", "insta", "regex", "tempfile"]

[dev-dependencies]
dunce = { workspace = true }
insta = { workspace = true, features = ["filters", "json"] }
regex = { workspace = true }
tempfile = { workspace = true }
Comment thread
dhruvmanila marked this conversation as resolved.
Outdated

Comment on lines +41 to +50

@dhruvmanila dhruvmanila Jul 24, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still required because we conditionally add the serde::Serialize attribute to the ClientOptions and other nested structs.

@MichaReiser MichaReiser Jul 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use assert_debug_snapshot instead so that we don't need the serde serialization?

@dhruvmanila dhruvmanila Jul 24, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not pretty ;)

@MichaReiser MichaReiser Jul 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it? Isn't it almost the same as JSON (unless we add some manual implementations):

https://insta.rs/docs/serializers/#debug

@MichaReiser MichaReiser Jul 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also be fine to simply always derive Serialize for those types. It's not that many and it simplifies the crate setup

@dhruvmanila dhruvmanila Jul 24, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think implementing Serialize unconditionally seems fine, I'll do it as a quick follow up tomorrow morning.

[target.'cfg(target_vendor = "apple")'.dependencies]
libc = { workspace = true }
Expand Down
3 changes: 2 additions & 1 deletion crates/ty_server/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use lsp_server::Connection;
use ruff_db::system::{OsSystem, SystemPathBuf};

use crate::server::Server;
pub use crate::session::ClientOptions;
Comment on lines +7 to +9

@dhruvmanila dhruvmanila Jul 24, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are required by the mock server implementation.

pub use document::{NotebookDocument, PositionEncoding, TextDocument};
pub(crate) use session::{DocumentQuery, Session};

Expand All @@ -14,7 +15,7 @@ mod server;
mod session;
mod system;

#[cfg(test)]
#[cfg(feature = "testing")]
pub mod test;

@MichaReiser MichaReiser Jul 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this implementation into the test folder too. See how I shared the comment CliTest infrastructure in ty/tests/cli

@dhruvmanila dhruvmanila Jul 24, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed this change. I've moved the tests under e2e/ directory and moved the TestServer and related data structures to e2e/main.rs file.


pub(crate) const SERVER_NAME: &str = "ty";
Expand Down
86 changes: 0 additions & 86 deletions crates/ty_server/src/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,89 +302,3 @@ impl Drop for ServerPanicHookHandler {
}
}
}

#[cfg(test)]
mod tests {
use anyhow::Result;
use lsp_types::notification::PublishDiagnostics;
use ruff_db::system::SystemPath;

use crate::session::ClientOptions;
use crate::test::TestServerBuilder;

#[test]
fn initialization() -> Result<()> {
let server = TestServerBuilder::new()?
.build()?
.wait_until_workspaces_are_initialized()?;

let initialization_result = server.initialization_result().unwrap();

insta::assert_json_snapshot!("initialization", initialization_result);

Ok(())
}

#[test]
fn initialization_with_workspace() -> Result<()> {
let workspace_root = SystemPath::new("foo");
let server = TestServerBuilder::new()?
.with_workspace(workspace_root, ClientOptions::default())?
.build()?
.wait_until_workspaces_are_initialized()?;

let initialization_result = server.initialization_result().unwrap();

insta::assert_json_snapshot!("initialization_with_workspace", initialization_result);

Ok(())
}

#[test]
fn publish_diagnostics_on_did_open() -> Result<()> {
let workspace_root = SystemPath::new("src");
let foo = SystemPath::new("src/foo.py");
let foo_content = "\
def foo() -> str:
return 42
";

let mut server = TestServerBuilder::new()?
.with_workspace(workspace_root, ClientOptions::default())?
.with_file(foo, foo_content)?
.enable_pull_diagnostics(false)
.build()?
.wait_until_workspaces_are_initialized()?;

server.open_text_document(foo, &foo_content, 1);
let diagnostics = server.await_notification::<PublishDiagnostics>()?;

insta::assert_debug_snapshot!(diagnostics);

Ok(())
}

#[test]
fn pull_diagnostics_on_did_open() -> Result<()> {
let workspace_root = SystemPath::new("src");
let foo = SystemPath::new("src/foo.py");
let foo_content = "\
def foo() -> str:
return 42
";

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()?;

server.open_text_document(foo, &foo_content, 1);
let diagnostics = server.document_diagnostic_request(foo)?;

insta::assert_debug_snapshot!(diagnostics);

Ok(())
}
}
3 changes: 2 additions & 1 deletion crates/ty_server/src/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ use ty_project::{ChangeResult, Db as _, ProjectDatabase, ProjectMetadata};

pub(crate) use self::capabilities::ResolvedClientCapabilities;
pub(crate) use self::index::DocumentQuery;
pub(crate) use self::options::{AllOptions, ClientOptions, DiagnosticMode};
pub use self::options::ClientOptions;
pub(crate) use self::options::{AllOptions, DiagnosticMode};
pub(crate) use self::settings::ClientSettings;
use crate::document::{DocumentKey, DocumentVersion, NotebookDocument};
use crate::server::publish_settings_diagnostics;
Expand Down
29 changes: 19 additions & 10 deletions crates/ty_server/src/session/options.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,10 @@ struct WorkspaceOptions {

/// This is a direct representation of the settings schema sent by the client.
#[derive(Clone, Debug, Deserialize, Default)]
#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))]
#[cfg_attr(test, derive(PartialEq, Eq))]
#[cfg_attr(feature = "testing", derive(serde::Serialize))]
#[serde(rename_all = "camelCase")]
pub(crate) struct ClientOptions {
pub struct ClientOptions {
/// Settings under the `python.*` namespace in VS Code that are useful for the ty language
/// server.
python: Option<Python>,
Expand All @@ -63,7 +64,8 @@ pub(crate) struct ClientOptions {

/// Diagnostic mode for the language server.
#[derive(Clone, Copy, Debug, Default, Deserialize)]
#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))]
#[cfg_attr(test, derive(PartialEq, Eq))]
#[cfg_attr(feature = "testing", derive(serde::Serialize))]
#[serde(rename_all = "camelCase")]
pub(crate) enum DiagnosticMode {
/// Check only currently open files.
Expand Down Expand Up @@ -147,21 +149,24 @@ impl ClientOptions {
// all settings and not just the ones in "python.*".

#[derive(Clone, Debug, Deserialize, Default)]
#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))]
#[cfg_attr(test, derive(PartialEq, Eq))]
#[cfg_attr(feature = "testing", derive(serde::Serialize))]
#[serde(rename_all = "camelCase")]
struct Python {
ty: Option<Ty>,
}

#[derive(Clone, Debug, Deserialize, Default)]
#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))]
#[cfg_attr(test, derive(PartialEq, Eq))]
#[cfg_attr(feature = "testing", derive(serde::Serialize))]
#[serde(rename_all = "camelCase")]
struct PythonExtension {
active_environment: Option<ActiveEnvironment>,
}

#[derive(Clone, Debug, Deserialize)]
#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))]
#[cfg_attr(test, derive(PartialEq, Eq))]
#[cfg_attr(feature = "testing", derive(serde::Serialize))]
#[serde(rename_all = "camelCase")]
pub(crate) struct ActiveEnvironment {
pub(crate) executable: PythonExecutable,
Expand All @@ -170,7 +175,8 @@ pub(crate) struct ActiveEnvironment {
}

#[derive(Clone, Debug, Deserialize)]
#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))]
#[cfg_attr(test, derive(PartialEq, Eq))]
#[cfg_attr(feature = "testing", derive(serde::Serialize))]
#[serde(rename_all = "camelCase")]
pub(crate) struct EnvironmentVersion {
pub(crate) major: i64,
Expand All @@ -182,7 +188,8 @@ pub(crate) struct EnvironmentVersion {
}

#[derive(Clone, Debug, Deserialize)]
#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))]
#[cfg_attr(test, derive(PartialEq, Eq))]
#[cfg_attr(feature = "testing", derive(serde::Serialize))]
#[serde(rename_all = "camelCase")]
pub(crate) struct PythonEnvironment {
pub(crate) folder_uri: Url,
Expand All @@ -194,7 +201,8 @@ pub(crate) struct PythonEnvironment {
}

#[derive(Clone, Debug, Deserialize)]
#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))]
#[cfg_attr(test, derive(PartialEq, Eq))]
#[cfg_attr(feature = "testing", derive(serde::Serialize))]
#[serde(rename_all = "camelCase")]
pub(crate) struct PythonExecutable {
#[allow(dead_code)]
Expand All @@ -203,7 +211,8 @@ pub(crate) struct PythonExecutable {
}

#[derive(Clone, Debug, Deserialize, Default)]
#[cfg_attr(test, derive(serde::Serialize, PartialEq, Eq))]
#[cfg_attr(test, derive(PartialEq, Eq))]
#[cfg_attr(feature = "testing", derive(serde::Serialize))]
#[serde(rename_all = "camelCase")]
struct Ty {
disable_language_services: Option<bool>,
Expand Down
Loading
Loading