Skip to content

test(download): serialize tests with proxy-sensitive URLs - #4372

Merged
rami3l merged 1 commit into
rust-lang:masterfrom
cuviper:test-proxies
Jun 6, 2025
Merged

test(download): serialize tests with proxy-sensitive URLs#4372
rami3l merged 1 commit into
rust-lang:masterfrom
cuviper:test-proxies

Conversation

@cuviper

Copy link
Copy Markdown
Member

There was already a guard for tests that set a proxy variable in the
environment, but this is also relevant for tests that would implicitly
read those variables while downloading a local serve_file URL.

@cuviper

cuviper commented Jun 5, 2025

Copy link
Copy Markdown
MemberAuthor

Without this change, I could pretty reliably induce this race by narrowing the testsuite, so they're more likely to run in parallel.

$ cargo test --features test --lib -- download::tests::
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.07s
Running unittests src/lib.rs (target/debug/deps/rustup-d6143b76edd93be0)
running 6 tests
test download::tests::curl::partially_downloaded_file_gets_resumed_from_byte_offset ... ok
test download::tests::curl::callback_gets_all_data_as_if_the_download_happened_all_at_once ... FAILED
test download::tests::reqwest::resume_partial_from_file_url ... ok
test download::tests::reqwest::callback_gets_all_data_as_if_the_download_happened_all_at_once ... FAILED
test download::tests::reqwest::socks_proxy_request ... FAILED
test download::tests::reqwest::read_basic_proxy_params ... ok
failures:
---- download::tests::curl::callback_gets_all_data_as_if_the_download_happened_all_at_once stdout ----
thread 'download::tests::curl::callback_gets_all_data_as_if_the_download_happened_all_at_once' panicked at src/download/tests.rs:85:14:
Test download failed: error during download
Caused by:
[97] proxy handshake error (connection to proxy closed)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- download::tests::reqwest::callback_gets_all_data_as_if_the_download_happened_all_at_once stdout ----
thread 'download::tests::reqwest::callback_gets_all_data_as_if_the_download_happened_all_at_once' panicked at src/download/tests.rs:252:14:
Test download failed: error downloading file
Caused by:
0: error sending request
1: client error (Connect)
2: socks connect error: unexpected end of file
---- download::tests::reqwest::socks_proxy_request stdout ----
thread 'download::tests::reqwest::socks_proxy_request' panicked at src/download/tests.rs:187:13:
assertion `left == right` failed
left: 3
right: 1
failures:
download::tests::curl::callback_gets_all_data_as_if_the_download_happened_all_at_once
download::tests::reqwest::callback_gets_all_data_as_if_the_download_happened_all_at_once
download::tests::reqwest::socks_proxy_request
test result: FAILED. 3 passed; 3 failed; 0 ignored; 0 measured; 114 filtered out; finished in 0.01s
error: test failed, to rerun pass `--lib`

Note that the socks_proxy_request saw 3 requests when it expected 1, because the two callback tests were also using that proxy environment variable.

@rami3l

Copy link
Copy Markdown
Member

@cuviper Thanks a lot for your investigation! This does look like one of the reasons why Rustup's CI is not as stable as expected. I'll have a look at your patch real soon :)

@rami3lrami3l self-assigned this Jun 6, 2025

@rami3lrami3l left a comment

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.

LGTM!

Nit: Before merging, it'd be probably better to change the commit message to "all download tests" instead of just "all tests"... I know by "proxy-sensitive URLs" it implies download tests, but it looks a bit misleading at the first glance 💦 Many thanks in advance I decided to do that myself, never mind :)

@rami3lrami3l changed the title Serialize all tests with proxy-sensitive URLsSerialize all download tests with proxy-sensitive URLsJun 6, 2025
@rami3lrami3l changed the title Serialize all download tests with proxy-sensitive URLstest(download): serialize tests with proxy-sensitive URLsJun 6, 2025
There was already a guard for tests that *set* a proxy variable in the
environment, but this is also relevant for tests that would implicitly
read those variables while downloading a local `serve_file` URL.
@rami3l
rami3l enabled auto-merge June 6, 2025 07:33
@rami3l
rami3l added this pull request to the merge queue Jun 6, 2025
djc
djc approved these changes Jun 6, 2025

@djcdjc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, thanks!

Merged via the queue into rust-lang:master with commit ece5ff0Jun 6, 2025
@kevinmehallkevinmehall mentioned this pull request Dec 14, 2025
3 tasks
@rami3lrami3l mentioned this pull request May 7, 2026
cons-tan-tan added a commit to cons-tan-tan/dotfiles that referenced this pull request May 21, 2026
The upstream fix (rust-lang/rustup#4372) has been merged and is
included in rustup 1.29.0, which is already available in the
locked nixpkgs. The skip-test workaround for
NixOS/nixpkgs#470496 is no longer needed; the package builds
cleanly and is fetched from cache.nixos.org.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@cuviper@rami3l@djc