Skip to content

[ty] Add panic-by-default await methods to TestServer#21451

Merged
MichaReiser merged 2 commits intomainfrom
micha/test-server-panic-by-default
Nov 14, 2025
Merged

[ty] Add panic-by-default await methods to TestServer#21451
MichaReiser merged 2 commits intomainfrom
micha/test-server-panic-by-default

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Nov 14, 2025

Summary

This should help with astral-sh/ty#1551. It doesn't
fix the root cause but I think it's an overall improvement to the testing experience.

The await_response, await_notification and await_request all return a Result today
to account for that they can fail. However, almost all writen E2E tests only care about the
case where a request is successful.

This PR changes the await_ methods to panic by default when they fail and adds
try_ counterparts that return a Result instead for the few cases where we
explicitly test the error case.

I find that this overall gives better ercognomis as we get a panic
on the line where the request failed rather than a: Test failed because request timed out
and you then have to guess which request it was.

Test Plan

cargo test

@MichaReiser MichaReiser requested a review from carljm as a code owner November 14, 2025 13:11
@MichaReiser MichaReiser added the testing Related to testing Ruff itself label Nov 14, 2025
@MichaReiser MichaReiser requested a review from sharkdp as a code owner November 14, 2025 13:11
@MichaReiser MichaReiser added the ty Multi-file analysis & type inference label Nov 14, 2025
@astral-sh-bot
Copy link

astral-sh-bot bot commented Nov 14, 2025 •
edited
Loading

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

@sharkdp sharkdp removed their request for review November 14, 2025 13:14
@astral-sh-bot
Copy link

astral-sh-bot bot commented Nov 14, 2025 •
edited
Loading

mypy_primer results

Changes were detected when running on open source projects 
scikit-build-core (https://github.com/scikit-build/scikit-build-core)
+ src/scikit_build_core/build/wheel.py:98:20: error[no-matching-overload] No overload of bound method `__init__` matches arguments
- Found 43 diagnostics
+ Found 44 diagnostics

No memory usage changes detected ✅

@MichaReiser MichaReiser force-pushed the micha/test-server-panic-by-default branch from 70f6a11 to 37f4080 Compare November 14, 2025 13:18
@MichaReiser MichaReiser requested review from dhruvmanila and removed request for carljm and dcreager November 14, 2025 13:18
dhruvmanila
dhruvmanila approved these changes Nov 14, 2025
View reviewed changes
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

This sounds reasonable to me, I only have a few minor comments which doesn't necessarily need to be addressed.

crates/ty_server/tests/e2e/main.rs Outdated
Comment on lines 390 to 391
Err(err) => match err {
TestServerError::ResponseError(response_error) => {
Copy link
Member

@dhruvmanila dhruvmanila Nov 14, 2025

Choose a reason for hiding this comment

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

Should we use the thiserror::Error messages here to display the error here? We could possibly add the request ID as context.

crates/ty_server/tests/e2e/main.rs Outdated
Comment on lines 481 to 482
Err(err) => match err {
TestServerError::RecvTimeoutError(RecvTimeoutError::Disconnected) => {
Copy link
Member

@dhruvmanila dhruvmanila Nov 14, 2025

Choose a reason for hiding this comment

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

Same here although this is a bit different but the response error variant could be moved first to use unreachable and then a catch-all err for the remaining error.

Copy link
Member Author

@MichaReiser MichaReiser Nov 14, 2025

Choose a reason for hiding this comment

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

Good idea

crates/ty_server/tests/e2e/main.rs Outdated
Comment on lines 585 to 586
Err(err) => match err {
TestServerError::RecvTimeoutError(RecvTimeoutError::Disconnected) => {
Copy link
Member

@dhruvmanila dhruvmanila Nov 14, 2025

Choose a reason for hiding this comment

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

Same here as the await_response comment.

@dhruvmanila
Copy link
Member

dhruvmanila commented Nov 14, 2025

I believe the return type for all of the test cases can now be removed (it is Result<()> currently) and the dangling Ok(()) after every test case as the panic is now part of the test server.

@MichaReiser
Copy link
Member Author

MichaReiser commented Nov 14, 2025

I believe the return type for all of the test cases can now be removed (it is Result<()> currently) and the dangling Ok(()) after every test case as the panic is now part of the test server.

Unfortunately not. TestServerBuilder::new can still fail for some reason (but that's fine because the drop handler will never execute in that case)

@MichaReiser MichaReiser force-pushed the micha/test-server-panic-by-default branch from c0abee1 to 1523b8d Compare November 14, 2025 18:32
@MichaReiser MichaReiser force-pushed the micha/test-server-panic-by-default branch from 1523b8d to 6e4b15b Compare November 14, 2025 18:34
@astral-sh-bot
Copy link

astral-sh-bot bot commented Nov 14, 2025

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 merged commit 008e9d0 into main Nov 14, 2025
41 checks passed
@MichaReiser MichaReiser deleted the micha/test-server-panic-by-default branch November 14, 2025 18:47
dcreager added a commit that referenced this pull request Nov 14, 2025
@dcreager
Merge remote-tracking branch 'origin/main' into dcreager/deep-comparison
f83a5a3 
* origin/main: (59 commits)
  [ty] Improve diagnostic range for `non-subscriptable` diagnostics (#21461)
  [ty] Improve literal promotion heuristics (#21439)
  [ty] Further improve details around which expressions should be deferred in stub files (#21456)
  [ty] Improve generic class constructor inference (#21442)
  [ty] Propagate type context through conditional expressions (#21443)
  [ty] Suppress completions when introducing names with `as`
  [ty] Add panic-by-default await methods to `TestServer` (#21451)
  [ty] name is parameter and global is a syntax error (#21312)
  [ty] Fixup a few details around version-specific dataclass features (#21453)
  [ty] Support attribute-expression `TYPE_CHECKING` conditionals (#21449)
  [ty] Support stringified annotations in value-position `Annotated` instances (#21447)
  [ty] Type inference for genererator expressions (#21437)
  [ty] Make `__getattr__` available for `ModuleType` instances (#21450)
  [ty] Increase default receive timeout in tests to 10s (#21448)
  [ty] Add synthetic members to completions on dataclasses (#21446)
  [ty] Support legacy `typing` special forms in implicit type aliases (#21433)
  Bump 0.14.5 (#21435)
  [ty] Support `type[…]` and `Type[…]` in implicit type aliases (#21421)
  [ty] Respect notebook cell boundaries when adding an auto import (#21322)
  Update PyCharm setup instructions (#21409)
  ...
dcreager added a commit that referenced this pull request Nov 14, 2025
@dcreager
Merge branch 'dcreager/deep-comparison' into dcreager/coolable
94106a4 
* dcreager/deep-comparison: (64 commits)
  assuming
  SubtypingAssuming
  implies_subtype_of
  name tweak
  Apply suggestions from code review
  [ty] Improve diagnostic range for `non-subscriptable` diagnostics (#21461)
  [ty] Improve literal promotion heuristics (#21439)
  [ty] Further improve details around which expressions should be deferred in stub files (#21456)
  [ty] Improve generic class constructor inference (#21442)
  [ty] Propagate type context through conditional expressions (#21443)
  [ty] Suppress completions when introducing names with `as`
  [ty] Add panic-by-default await methods to `TestServer` (#21451)
  [ty] name is parameter and global is a syntax error (#21312)
  [ty] Fixup a few details around version-specific dataclass features (#21453)
  [ty] Support attribute-expression `TYPE_CHECKING` conditionals (#21449)
  [ty] Support stringified annotations in value-position `Annotated` instances (#21447)
  [ty] Type inference for genererator expressions (#21437)
  [ty] Make `__getattr__` available for `ModuleType` instances (#21450)
  [ty] Increase default receive timeout in tests to 10s (#21448)
  [ty] Add synthetic members to completions on dataclasses (#21446)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhruvmanila dhruvmanila dhruvmanila approved these changes

Assignees

No one assigned

Labels

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.

2 participants

@MichaReiser @dhruvmanila