test: Ensure we can read proxy params from environment #2407
Conversation
inejge
force-pushed
the
master
branch
2 times, most recently
from
d67b839 to
eae405c
Compare
|
OK, slightly modified to deal with variable name case insensitivity on Windows. The reason for the macOS build failure is unclear to me. |
|
That lock is only ever used in that test, so I'm not sure how useful it is. Better would be to ensure we restore the environment after with a guard like the ones used in the integration tests, or else for The restoration is important otherwise this will probably mess up developer boxes which set environment variables for proxying on Windows. |
No objection there, it can be removed.
Those guards restore |
|
Aha, I'm a little sensitive about environment variables in Windows as you might imagine :D Okay. (The macos failure was a GHA bogon btw) |
| set_var("HTTPS_PROXY", "http://proxy2.example.com:8080"); | ||
| let u = Url::parse("https://www.example.org").ok().unwrap(); | ||
| assert_eq!( | ||
| for_url(&u).host_port(), |
There was a problem hiding this comment.
This is using the env_proxy function directly: I guess thats what broke, so we probably need to, but its a bit of a shame to be testing our dependencies :(. I haven't looked but if we wrap things up in higher order things maybe those should be checked instead/too.
There was a problem hiding this comment.
Looking at the antecedents of #2399, it should be clear that env_proxy's own logging and tests did detect the breakage, but it wouldn't have helped here; the even higher order problem stemmed from my failure to publish the updated version in a timely manner.
I agree that a higher-level test would be architecturally cleaner, and could imagine spinning up a separate micro-proxy thread (could it be backed by hyper? Probably) and attempting to fetch a file through that. In any case, proxies are an important enough use case for rustup that I believe that they should be tested for in some manner.
| @@ -0,0 +1,50 @@ | |||
| #![cfg(feature = "reqwest-backend")] | |||
There was a problem hiding this comment.
Did this bug only affect one backend?
There was a problem hiding this comment.
Yes, cURL has its own proxy detection logic (which env_proxy emulates).
| @@ -0,0 +1,50 @@ | |||
| #![cfg(feature = "reqwest-backend")] | |||
|
|
|||
There was a problem hiding this comment.
separately, this is going to make a separate integration test binary; rather than a unit test. I think this is ok for now, but having it in a larger compilation unit would help prevent proliferation, but we'll need to integrate in with currentprocess() to do that - I haven't looked at env_proxy to see whats involved there.
An alternative approach would be to have a helper that will always be isolated (like this is), but simpler: don't scrub the environment or anything; just print host_port() to stdout; then when we run this helper binary, we can set the environment, allowing us to run different scenarios.
But I don't think we need that approach yet, since what we're doing here is really just a smoke test.
download/tests/read-proxy-env.rs
Outdated
| use url::Url; | ||
|
|
||
| lazy_static! { | ||
| static ref LOCK: Mutex<()> = Mutex::new(()); |
There was a problem hiding this comment.
I don't think the lock is of any use right now - we have one and only one test, in one test script, and nothing else is consulting it, so it isn't going to lock anything out: just ditch it.
download/tests/read-proxy-env.rs
Outdated
|
|
||
| // Tests for: | ||
| // 1. Correctly retrieving the proxy (host, port) tuple from $https_proxy or $HTTPS_PROXY | ||
| // 2. Prioritizing $https_proxy over $HTTPS_PROXY (except on Windows, where env vars |
There was a problem hiding this comment.
I don't think point 2 is interesting: this is very much in the space of 'checking env_proxy internals' : the point of this test is to make sure that urls hooks <-> env_proxy are setup properly in case of future dependency skew issues, not to test the definition of env-proxy.
There was a problem hiding this comment.
Ack, just checking https_proxy is probably enough.
|
To summarize, the way forward (in the near term, for this PR) would be to: a) remove locking, b) limit the test to |
In general, it's easier to review changes-in-response-to-review if you push new commits to do the change, though if you and @rbtcollins are in agreement on the way forward then jumping immediately to a force-push is fine. I definitely prefer that a PR be rebased to be clean commits before merging though. |
|
Right, here is the updated test as an additional commit, then. |
|
LGTM, please squash into a single logical commit to prep for merging |
Tests for correctly retrieving the proxy (host, port) tuple from $https_proxy by directly calling env_proxy::for_url()
|
Rebased and force-pushed. |
Tests for:
Correctly retrieving the proxy
(host, port)tuple from$https_proxyor$HTTPS_PROXYPrioritizing
$https_proxyover$HTTPS_PROXYUsing a mutex to serialize environment access in the test function is not strictly necessary as long as there are no other tests running in parallel, but makes it easier to extend testing if the need arises.