Add a timeout to the `remote-test-client` connection #147952

pvdrz · 2025-10-21T23:10:58Z

Currently, the remote-test-client doesn't have a timeout when connecting to the remote-test-server. This means that running tests using it can hang indefinitely which causes issues when running tests on CI, for example.

This PR now sets a default timeout of 5 minutes, meaning that if, for example, TEST_DEVICE_ADDR=<IP:PORT> ./x test --target riscv64gc-unknown-linux-gnu tests/ui is run and the remote-test-server is not reachable by the client, the client will panic after the timeout is reached.

Additionally, the TEST_DEVICE_CONNECT_TIMEOUT env variable can be used to set up the timeout to any value (in seconds).

This PR also wires up a test step for remote-test-client, which didn't previously have tool tests run in CI.

rustbot · 2025-10-21T23:11:03Z

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

src/tools/remote-test-client/src/main.rs

jyn514

LGTM but I no longer have r+ permissions

View changes since this review

src/tools/remote-test-client/src/main.rs

Enselic · 2025-10-25T12:51:30Z

src/tools/remote-test-client/src/main.rs

+        total_dur += dur;
    }
+
+    panic!("Test device at {device_address} timed out");


It would be comforting with a test for this. I'm thinking a test where we make the binary connect to non-existing address and use a small timeout and assert on that it panics.

src/tools/remote-test-client/src/main.rs

Enselic · 2025-10-29T05:00:42Z

I'll assign myself to this one for now.

r? Enselic

Enselic

Found some more things. To avoid confusion for the test later: I would prefer a test that actually invokes the binary rather than having some kind of unit test.

View changes since this review

src/tools/remote-test-client/src/main.rs

Enselic · 2025-10-31T18:46:49Z

I'm currently happy with the way the code looks. So just a test left as far as I'm concerned.

pvdrz · 2025-11-04T15:38:55Z

@Enselic Do you want me to factor out the main function so I can write a unit test for it or do you want me to write a test that actually runs the binary? If you want the latter, I don't know how to do it so I'd need some instructions.

Enselic · 2025-11-05T04:47:29Z

A test that actually runs the binary. I can see that we've had assert_cmd in our Cargo.lock before (added with #125408), so let's see how it feels to use assert_cmd to test the remote-test-client binary. Let's use the same structure as src/tools/jsondoclint/src/validator/tests.rs and put tests.rs under src. I think that helps with build dependency resolution and minimize rebuilds.

Another thing: We need to document the new environment variable in src/doc/rustc-dev-guide/src/tests/running.md.

rustbot · 2025-11-06T05:36:46Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

rustbot · 2025-11-06T05:41:06Z

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol, @tshepang

rustbot · 2025-11-08T15:53:21Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

jyn514 · 2025-11-10T15:48:23Z

@Enselic is it necessary to land it as a separate PR? it seems quite tied to the changes in this one, it would suck to land a separate PR only to find out that the tests here needed further changes to the test infrastructure.

in general I feel like this PR is being held to a higher standard than most changes to remote-test-client. my understanding is that it didn't have a test suite at all before this change? it seems like a strict improvement to have an ok test suite and a ok tests rather than no tests at all.

Enselic · 2025-11-10T21:01:45Z

Well, reviewer guidelines say that stuff should be tested. And that PRs should be as small as possible. That is also an approach I personally think is preferred. So for me it seems natural to split.

But I am completely fine with others doing r+ on this if they are happy with the PR.

jieyouxu

I'm fine with landing these together given it's not too large.

View changes since this review

jieyouxu · 2025-11-11T00:49:41Z

Well, reviewer guidelines say that stuff should be tested. And that PRs should be as small as possible. That is also an approach I personally think is preferred.

It's a judgement call, would definitely also ask for a split if the test infra changes were larger.

jieyouxu · 2025-11-11T00:50:31Z

I edited the PR description to reflect that this PR is also registering a test step for remote-test-client.

@bors r=Enselic,jieyouxu rollup

bors · 2025-11-11T00:50:34Z

📌 Commit bd14144 has been approved by Enselic,jieyouxu

It is now in the queue for this repository.

…-timeout, r=Enselic,jieyouxu Add a timeout to the `remote-test-client` connection Currently, the `remote-test-client` doesn't have a timeout when connecting to the `remote-test-server`. This means that running tests using it can hang indefinitely which causes issues when running tests on CI, for example. This PR now sets a default timeout of 5 minutes, meaning that if, for example, `TEST_DEVICE_ADDR=<IP:PORT> ./x test --target riscv64gc-unknown-linux-gnu tests/ui` is run and the `remote-test-server` is not reachable by the client, the client will panic after the timeout is reached. Additionally, the `TEST_DEVICE_CONNECT_TIMEOUT` env variable can be used to set up the timeout to any value (in seconds). This PR also wires up a test step for `remote-test-client`, which didn't previously have tool tests run in CI.

Rollup of 16 pull requests Successful merges: - #141470 (Add new `function_casts_as_integer` lint) - #143619 (`c_variadic`: Add future-incompatibility warning for `...` arguments without a pattern outside of `extern` blocks) - #146495 (rustdoc: Erase `#![doc(document_private_items)]`) - #147771 (Rename `*exact_{div,shr,shl}` to `*{div,shr,shl}_exact`) - #147833 (rustdoc-json: move `target` to `json::conversions`) - #147952 (Add a timeout to the `remote-test-client` connection) - #147955 (compiletest: Migrate `TestProps` directive handling to a system of named handlers) - #148480 (Add `Steal::risky_hack_borrow_mut`) - #148506 (Special case detecting `'static` lifetime requirement coming from `-> Box<dyn Trait>`) - #148508 (Provide more context when mutably borrowing an imutably borrowed value) - #148530 (update the bootstrap readme) - #148608 (Add test for --test-builder success path) - #148636 (bootstrap: respect `build.python` on macOS) - #148639 (test(rustdoc): move tests into jump-to-def) - #148647 (Check unsafety for non-macro attributes in `validate_attr`) - #148667 (a few small clippy fixes) r? `@ghost` `@rustbot` modify labels: rollup

Enselic · 2025-11-11T08:37:57Z

Since I was partly put as r on this I'd like to get rid of the two "Adress review comments" commits before merge (and it's not just me: that is also part of reviewer guidelines). This PR should be at most 2 commits. One to add test support and one to add the feature. I'm also fine with just one commit.

@bors r-

Rollup of 16 pull requests Successful merges: - #141470 (Add new `function_casts_as_integer` lint) - #143619 (`c_variadic`: Add future-incompatibility warning for `...` arguments without a pattern outside of `extern` blocks) - #146495 (rustdoc: Erase `#![doc(document_private_items)]`) - #147771 (Rename `*exact_{div,shr,shl}` to `*{div,shr,shl}_exact`) - #147833 (rustdoc-json: move `target` to `json::conversions`) - #147952 (Add a timeout to the `remote-test-client` connection) - #147955 (compiletest: Migrate `TestProps` directive handling to a system of named handlers) - #148480 (Add `Steal::risky_hack_borrow_mut`) - #148506 (Special case detecting `'static` lifetime requirement coming from `-> Box<dyn Trait>`) - #148508 (Provide more context when mutably borrowing an imutably borrowed value) - #148530 (update the bootstrap readme) - #148608 (Add test for --test-builder success path) - #148636 (bootstrap: respect `build.python` on macOS) - #148639 (test(rustdoc): move tests into jump-to-def) - #148647 (Check unsafety for non-macro attributes in `validate_attr`) - #148667 (a few small clippy fixes) r? `@ghost` `@rustbot` modify labels: rollup

pvdrz · 2025-11-14T16:08:56Z

@Enselic I squashed the commits

Enselic · 2025-11-14T17:36:10Z

I'm not sure I am the best person to review this in its current form? In order for me to r this I'd need:

A. the new test step in a separate PR, since I don't know enough about bootstrap to r it.
B. formal blessing from T-bootstrap that adding a new env var (which will be a long term public API to remote-test-client) is OK
C. iterate on the test code more

A and B was was r:ed by jieyouxu already, and for C I understand if people think I am too picky, so maybe best if you review this again @jieyouxu ?

Just don't add me to r= and I will keep quiet :) Note: I'm guessing you did it the first time out of courtesy, and if so I appreciate the intention!

jieyouxu · 2025-11-15T08:30:17Z

A. the new test step in a separate PR, since I don't know enough about bootstrap to r it.

This is fine in this PR. Or at least, I reviewed the bootstrap changes (with a bootstrap reviewer hat on) and it LGTM.

B. formal blessing from T-bootstrap that adding a new env var (which will be a long term public API to remote-test-client) is OK

This is fine too, since remote-test-client is not "user-facing", it's an "internal" test support tool. It's even fine if we revert/remove that after it's added, because there's no stability promise here.

C. iterate on the test code more

If you'd like to iterate on the test code more, that's fine by me (my interest was only in the test step not this test tool specifically). I was only here to request to not add more special path mapping to test::CrateBootstrap :D

IMO there are two ways to go about this:

We land this intermediate version as-is, some follow-up changes to remote-test-client could be in another PR (again, there's no stability promise for the test tool, so feel free to iterate the API all you want as long as it's still testing things).
We split the bootstrap test step infra changes out to a separate PR (I can review and r+ that), then I'll leave it to you fellas to iterate on the test tool itself.

(I wasn't looking to review the changes to remote-test-client itself.)

@rustbot review

Enselic · 2025-11-15T11:22:06Z

Thank you for elaborating. Option 2 will be best IMHO.

rustbot assigned Mark-Simulacrum Oct 21, 2025

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2025

jyn514 reviewed Oct 22, 2025

View reviewed changes

src/tools/remote-test-client/src/main.rs Outdated Show resolved Hide resolved

pvdrz force-pushed the pvdrz/remote-test-client-timeout branch from 1d1f5fd to 799df1c Compare October 22, 2025 14:50

jyn514 approved these changes Oct 22, 2025

View reviewed changes

Enselic requested changes Oct 25, 2025

View reviewed changes

pvdrz requested a review from Enselic October 27, 2025 15:49

This comment has been minimized.

Sign in to view

Enselic requested changes Oct 29, 2025

View reviewed changes

src/tools/remote-test-client/src/main.rs Outdated Show resolved Hide resolved

src/tools/remote-test-client/src/main.rs Show resolved Hide resolved

src/tools/remote-test-client/src/main.rs Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

rustbot assigned Enselic and unassigned Mark-Simulacrum Oct 29, 2025

pvdrz requested a review from Enselic October 29, 2025 16:02

Enselic requested changes Oct 30, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2025

pvdrz force-pushed the pvdrz/remote-test-client-timeout branch from b79af79 to 7a6c157 Compare October 30, 2025 18:44

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Nov 6, 2025

rustbot added the A-rustc-dev-guide Area: rustc-dev-guide label Nov 6, 2025

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 8, 2025

jieyouxu approved these changes Nov 11, 2025

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 11, 2025

Zalathar mentioned this pull request Nov 11, 2025

Rollup of 17 pull requests #148810

Closed

Zalathar mentioned this pull request Nov 11, 2025

Rollup of 16 pull requests #148813

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 11, 2025

jieyouxu removed their assignment Nov 11, 2025

pvdrz added 2 commits November 11, 2025 12:15

Add a timeout to the remote-test-client connection

a73faa2

Document the new env variable in the rustc-dev-guide

08d3225

pvdrz force-pushed the pvdrz/remote-test-client-timeout branch from bd14144 to 08d3225 Compare November 11, 2025 17:15

pvdrz requested a review from jieyouxu November 12, 2025 16:30

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 15, 2025

Add a timeout to the remote-test-client connection #147952

Are you sure you want to change the base?

Add a timeout to the remote-test-client connection #147952

Uh oh!

Conversation

pvdrz commented Oct 21, 2025 • edited by jieyouxu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 21, 2025

Uh oh!

Uh oh!

jyn514 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Enselic Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Enselic commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Enselic left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Enselic commented Oct 31, 2025

Uh oh!

pvdrz commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Enselic commented Nov 5, 2025

Uh oh!

rustbot commented Nov 6, 2025

Uh oh!

rustbot commented Nov 6, 2025

Uh oh!

rustbot commented Nov 8, 2025

Uh oh!

jyn514 commented Nov 10, 2025

Uh oh!

Enselic commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jieyouxu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Nov 11, 2025

Uh oh!

jieyouxu commented Nov 11, 2025

Uh oh!

bors commented Nov 11, 2025

Uh oh!

Enselic commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pvdrz commented Nov 14, 2025

Uh oh!

Enselic commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Add a timeout to the `remote-test-client` connection #147952

Add a timeout to the `remote-test-client` connection #147952

pvdrz commented Oct 21, 2025 •

edited by jieyouxu

Loading

jyn514 left a comment •

edited by rustbot

Loading

Enselic Oct 25, 2025 •

edited

Loading

Enselic commented Oct 29, 2025 •

edited

Loading

Enselic left a comment •

edited by rustbot

Loading

pvdrz commented Nov 4, 2025 •

edited

Loading

Enselic commented Nov 10, 2025 •

edited

Loading

jieyouxu left a comment •

edited by rustbot

Loading

Enselic commented Nov 11, 2025 •

edited

Loading

Enselic commented Nov 14, 2025 •

edited

Loading

jieyouxu commented Nov 15, 2025 •

edited

Loading