Skip to content

download: more simplifications#4848

Open
djc wants to merge 8 commits intomainfrom
rm-callback
Open

download: more simplifications#4848
djc wants to merge 8 commits intomainfrom
rm-callback

Conversation

@djc
Copy link
Copy Markdown
Contributor

@djc djc commented May 6, 2026

Following up to

Interestingly, it looks like a number of the tests time out on my macOS machine when run as part of a full cargo test --features test (even on main):

test download::tests::reqwest::network_failure_does_not_delete_partial_file has been running for over 60 seconds
test download::tests::reqwest::read_basic_proxy_params has been running for over 60 seconds
test download::tests::reqwest::resume_partial_fails_if_server_ignores_range has been running for over 60 seconds
test download::tests::reqwest::socks_proxy_request has been running for over 60 seconds
test download::tests::reqwest::network_failure_does_not_delete_partial_file ... ok

although they work okay when running cargo test --features test download::tests.

@djc djc requested review from FranciscoTGouveia and rami3l May 6, 2026 20:19
@rami3l rami3l self-assigned this May 7, 2026
@rami3l
Copy link
Copy Markdown
Member

rami3l commented May 7, 2026

Interestingly, it looks like a number of the tests time out on my macOS machine when run as part of a full cargo test --features test (even on main):

test download::tests::reqwest::network_failure_does_not_delete_partial_file has been running for over 60 seconds
test download::tests::reqwest::read_basic_proxy_params has been running for over 60 seconds
test download::tests::reqwest::resume_partial_fails_if_server_ignores_range has been running for over 60 seconds
test download::tests::reqwest::socks_proxy_request has been running for over 60 seconds
test download::tests::reqwest::network_failure_does_not_delete_partial_file ... ok

although they work okay when running cargo test --features test download::tests.

@djc Cannot reproduce on my machine here. Do you happen to have HTTP_PROXY set? Because scrub_env() forgot to scrub it for some reason...

But it does make sense because any test currently relying on scrub_env() is behind a lock (#4372). One waits for another when other stuff is running... It is possible for this to get over 60s as long as they eventually pass.

Copy link
Copy Markdown
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

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

Many thanks for all the cleanup :D

View changes since this review

Comment thread src/download/tests.rs
assert_eq!(std::fs::read_to_string(&target_path).unwrap(), "12345");
}

#[tokio::test]
Copy link
Copy Markdown
Member

@rami3l rami3l May 7, 2026

Choose a reason for hiding this comment

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

Note to the future reviewer: the test right above is already a direct-style download resumption test, so it's safe to delete this one.

Sign up for free to 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.

2 participants