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

Jump to bottom
Merged
merged 3 commits into from
Jul 24, 2025
Merged

[ty] Move server tests as integration tests #19522

merged 3 commits into from
Jul 24, 2025

Conversation

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Jul 24, 2025

Summary

Reference: #19391 (comment)

@dhruvmanila dhruvmanila added the server Related to the LSP server label Jul 24, 2025
@dhruvmanila dhruvmanila added testing Related to testing Ruff itself ty Multi-file analysis & type inference labels Jul 24, 2025
dhruvmanila
dhruvmanila commented Jul 24, 2025
View reviewed changes
@github-actions
Copy link
Contributor

github-actions bot commented Jul 24, 2025 •
edited
Loading

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

MichaReiser
MichaReiser reviewed Jul 24, 2025
View reviewed changes
crates/ty_server/src/lib.rs Outdated
Comment on lines 18 to 19
#[cfg(feature = "testing")]
pub mod test;
Copy link
Member

@MichaReiser MichaReiser Jul 24, 2025

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

Copy link
Member Author

@dhruvmanila dhruvmanila Jul 24, 2025

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.

dhruvmanila
dhruvmanila commented Jul 24, 2025
View reviewed changes
crates/ty_server/Cargo.toml
Comment on lines +41 to +50
ty_server = { workspace = true, features = ["testing"] }

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

[features]
testing = []

Copy link
Member Author

@dhruvmanila dhruvmanila Jul 24, 2025

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.

Copy link
Member

@MichaReiser MichaReiser Jul 24, 2025

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?

Copy link
Member Author

@dhruvmanila dhruvmanila Jul 24, 2025

Choose a reason for hiding this comment

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

That is not pretty ;)

Copy link
Member

@MichaReiser MichaReiser Jul 24, 2025

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

Copy link
Member

@MichaReiser MichaReiser Jul 24, 2025

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

Copy link
Member Author

@dhruvmanila dhruvmanila Jul 24, 2025

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.

dhruvmanila
dhruvmanila commented Jul 24, 2025
View reviewed changes
crates/ty_server/src/lib.rs
Comment on lines +7 to +9
pub use crate::logging::{LogLevel, init_logging};
pub use crate::server::Server;
pub use crate::session::ClientOptions;
Copy link
Member Author

@dhruvmanila dhruvmanila Jul 24, 2025

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.

@dhruvmanila dhruvmanila requested a review from MichaReiser July 24, 2025 13:37
@github-actions
Copy link
Contributor

github-actions bot commented Jul 24, 2025 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

MichaReiser
MichaReiser approved these changes Jul 24, 2025
View reviewed changes
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thank you

crates/ty_server/Cargo.toml
Comment on lines +41 to +50
ty_server = { workspace = true, features = ["testing"] }

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

[features]
testing = []

Copy link
Member

@MichaReiser MichaReiser Jul 24, 2025

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 enabled auto-merge (squash) July 24, 2025 16:07
@dhruvmanila dhruvmanila merged commit f9091ea into main Jul 24, 2025
36 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/move-server-tests-as-integration branch July 24, 2025 16:10
UnboundVariable pushed a commit to UnboundVariable/ruff that referenced this pull request Jul 24, 2025
Merge branch 'main' of https://github.com/astral-sh/ruff into doc_hig…
1f937bd 
…hlight

* 'main' of https://github.com/astral-sh/ruff:
  [ty] Minor: fix incomplete docstring (astral-sh#19534)
  [ty] Move server tests as integration tests (astral-sh#19522)
  [`ruff`] Offer fixes for `RUF039` in more cases (astral-sh#19065)
  [ty] Support `dataclasses.InitVar` (astral-sh#19527)
  [`ruff`] Fix `RUF033` breaking with named default expressions (astral-sh#19115)
  Update pre-commit hook name (astral-sh#19530)
  Bump 0.12.5 (astral-sh#19528)
  [ty] Rename type_api => ty_extensions (astral-sh#19523)
  [ty] Added support for "go to references" in ty playground. (astral-sh#19516)
  [ty] Return a tuple spec from the iterator protocol (astral-sh#19496)
  [ty] Exhaustiveness checking & reachability for `match` statements (astral-sh#19508)
  [ty] Fix narrowing and reachability of class patterns with arguments (astral-sh#19512)
UnboundVariable pushed a commit to UnboundVariable/ruff that referenced this pull request Jul 24, 2025
Merge branch 'main' into symbols
9ddb01c 
* main:
  [ty] Added support for "document highlights" language server feature. (astral-sh#19515)
  Add support for specifying minimum dots in detected string imports (astral-sh#19538)
  [ty] Minor: fix incomplete docstring (astral-sh#19534)
  [ty] Move server tests as integration tests (astral-sh#19522)
  [`ruff`] Offer fixes for `RUF039` in more cases (astral-sh#19065)
  [ty] Support `dataclasses.InitVar` (astral-sh#19527)
  [`ruff`] Fix `RUF033` breaking with named default expressions (astral-sh#19115)
  Update pre-commit hook name (astral-sh#19530)
  Bump 0.12.5 (astral-sh#19528)
  [ty] Rename type_api => ty_extensions (astral-sh#19523)
  [ty] Added support for "go to references" in ty playground. (astral-sh#19516)

# Conflicts:
#	crates/ty_server/src/server/api/requests.rs
#	crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization.snap
#	crates/ty_server/tests/e2e/snapshots/e2e__initialize__initialization_with_workspace.snap
AlexWaygood pushed a commit that referenced this pull request Jul 25, 2025
@dhruvmanila @AlexWaygood
[ty] Move server tests as integration tests (#19522)
80a2469 
## Summary

Reference:
#19391 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

Assignees

No one assigned

Labels

server Related to the LSP server testing Related to testing Ruff itself ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dhruvmanila @MichaReiser