Re: [pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 16/09/2022 um 10:17 schrieb Stefan Hanreich:
> Yes, I was also not very happy with how I had to test it. If i really wanted to test it properly I'd add tests to the ProxyConfig as well as the HttpsConnector, but that seemed a bit out of scope for this patch series, which is why I resorted to this (seemingly) basic test.
> 
> It was more of a basic sanity check to see if I didn't break some stuff.

Sure, but test code should be near the thing actually tested. Note also that
if we would add https to ProxyConfig in the future it would fail a "distant"
crate in a bit confusing way (the "slightly confusing" part could probably be
avoided by checking either the actual error or a comment for why it's expected
to fail), all noticeable and thus fixable before rollout, so nothing _that bad_,
don't get me wrong, but it would Just Work™ otherwise.

I'm also a bit wondering how setting environment variables affects other tests in
general, iow. how isolated this all is. I'd not 100% sure if rust spawns a separate
process per test and also then it may be a bit cleaner to first get the current env
variable one is modifying and restoring that value at the end of the test. I don't
think that would be an issue here, as we shouldn't rely on any connections at all,
so more of a general wondering-thing.

> Do you think it would make sense to create a separate patch series that unit tests ProxyConfig with some (im)possible proxy settings?

The current ProxyConfig is 89 Lines with comments and generous extra lines
of rather straight forward code, so while a clear cut specific test definitively
wouldn't be hurting, investing to much time in any would probably amount to a
rather mediocre ROI. With that in mind: your choice, if you send a somewhat sensible
one I'll definitively apply it.