Add a timeout to the remote-test-client connection

[pvdrz]

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.

Edit: blocked by #149071

[rustbot]

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

[jyn514]

LGTM but I no longer have r+ permissions

View changes since this review

[Enselic]

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

[Enselic]

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

[pvdrz]

@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]

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]

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]

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

[pvdrz]

I have to say that having to split this PR in two after having squashed everything is not great. I'm still going to do it, just wanted to point that out.

[Enselic]

Thank you for creating a separate PR, and I understand that you don't think the back and forth has been a great experience. Rest assured that everyone is trying their best to navigate the various wills and tradeoffs at play here.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Enselic]

With rust-lang/rustc-dev-guide#2657 it is more "official" that adding TEST_DEVICE_CONNECT_TIMEOUT_SECONDS is not that big of a deal. And the code itself looks good to me now, thanks.

@bors r+

(My impression is that you do not expect to be part of r on this PR jieyouxu. Please let me know if I misunderstood.)

I am not going to insist on that you do another rebase, but I'm curious why you split up the implementation and the test as separate commits? That makes a lot of sense to do for bug fixes, since then you could end up wanting to revert the fix but keep the test. But for new features you never want to revert the fix and keep the test, so it makes sense to keep the fix and test in the same commit. And for git blame and git log, people will want to easily find the test that comes along with the fix. Which gets trickier if they are split.

But again, just wanted to mention this. I'm still going to approve this as you can see above.

View changes since this review

[Enselic]

TIL that bors does not listen to approval comments.

@bors r+

[bors]

📌 Commit 6bd5de7 has been approved by Enselic

It is now in the queue for this repository.

[pvdrz]

but I'm curious why you split up the implementation and the test as separate commits? That makes a lot of sense to do for bug fixes, since then you could end up wanting to revert the fix but keep the test. But for new features you never want to revert the fix and keep the test, so it makes sense to keep the fix and test in the same commit.

Yeah but if someone reverts the feature, they need to remind themselves that they changed the interface of the tool and that a test is broken because we expected that feature to exist. So having to also revert the test is a little safety belt so they don't break other people's workflow by accident.