Pass the session's proxies property to request

[junqfisica]

After waiting a long time for a fix on the side of the request that could fix pip install issues with proxy as already discussed in #9691. It seems that such changes as mentioned in psf/requests#5735 will take a while to happen, probably only on version 3 of the request.

Therefore, I'm suggesting a change on the pip side as already proposed in #9568 (comment) and psf/requests#5735 (comment)

I think it's important that pip address this issue asap since the newer versions of pip are not working properly behind a proxy.

[pfmoore]

This needs a test, to ensure that (a) it fixes the issue, and (b) we don't introduce regressions with future changes.

[pradyunsg]

Do we have tests for any sort of proxy logic? If not, it might be fine to merge this as-is. 💁🏽

That said, there’s no version of this where having an actual test written is problematic, so it would 100% be a good idea to have one here.

[pfmoore]

I don't know enough myself about proxy handling to be sure it's correct without a test, but I'm fine if someone else wants to merge it as is.

[junqfisica]

Hello @pfmoore and @pradyunsg

I've added (committed) a test_proxy in test_network_session.py.
However, you must provide a valid proxy so this test can pass, you could also reverse the logic and test a non-working proxy, but in my view, that would be a weak test.
On my end the test passes when I run with the modifications proposed in this commit, otherwise, it fails.
Feel free to change or even remove the test if providing a proxy proves to be a hassle. :)

@pfmoore I don't know what do you mean by "we don't introduce regressions with future changes.". What kind of regression is introduced by these changes? Would be nice if you could clarify this better. :). I hope this can help somehow to remove the bug with proxy introduced by request.

[pfmoore]

However, you must provide a valid proxy so this test can pass

That's not how the tests need to work. You'd need to have some sort of mock proxy as part of the test suite in order to ensure that people (or the CI!) without a proxy available can still run the test.

I don't know what do you mean by "we don't introduce regressions with future changes."

I mean that when someone else comes along and changes the networking code, we need to be sure that they don't break things again. The best (only) way to do that is if we have something in the test suite that checks the new code you added, and confirms it's working, Then if someone later breaks proxy support, the test suite will fail and they will find out.

[CrazyBoyFeng]

Simulating the behavior of a proxy is a complicated matter. If it is possible to import third-party libraries into the tests, the most efficient way is to create a real proxy server.

[junqfisica]

@pfmoore Thank you for your clarification.
I agree that some sort of server mocking would be better, but as already mentioned this is not an easy task. I would be glad to help if someone has an idea of how to do it, otherwise, you could just remove this test.

I'm sorry for the previous error, I just realized there was an indentation problem on the test committed since I just copy and paste my local changes directly on the raw file on Github. I will clone this branch so that doesn't happen again...sorry

[uranusjr]

The change looks good to me.

Can you add @pytest.mark.network to the test to signify the test relies on network? It allows skipping the test in non-internet-connected setups. (I think that’s the marker to use; please search the codebase to ensure correct spelling.)

Also please add a news entry for this.

[pfmoore]

Given the provided test will always fail on our CI, it's not much use IMO (anyone who can manually set a proxy to exercise the test can probably also set their proxy and just try pip out, after all...). My main intention in asking for a test was so that people without the ability to set up a proxy would be covered, because the test would run in CI to ensure this functionality still worked.

Question for the other @pypa/pip-committers - do we want to include this change without it being tested by our CI?

I'm ambivalent. The change is simple enough that asking for a test is making the whole PR turn into a much bigger exercise than it should be¹. But on the other hand, if we do ever switch to another network library like httpx, how will we know that proxy support still works if we have no test?

Footnotes

1. And @junqfisica thanks for being patient in working with us on this. ↩

[uranusjr]

I think we should; if we don’t want to add anything non-testable, we should’ve not supported proxies to begin with 🙂

[notatallshaw]

What about an integration test where a simple proxy is set up and then it tests that it can use it to say download?

I'm experimenting with it now, would you be interested with in a PR if I get it working okay?

[pradyunsg]

I take it that #10901 is the PR.

[notatallshaw]

I take it that #10901 is the PR.

Yes

[junqfisica]

Hi everyone,

I have updated the test_network_session and added the @pytest.mark.network as @uranusjr suggested. I also added a --proxy keyword so users can run the test with a given proxy as suggested by @pfmoore.

Now the test works as follow:

1. If the system has a proxy, the test tries to get it and it should pass.
2. If the system has no proxy the test also should pass as far as a proxy is not required by your network.
3. Users can pass a proxy to the test using the --proxy keyword, if a valid proxy is passed then the test should pass.

Examples of how to run:
Should Pass if the system has a valid proxy or no proxy.
pytest -s tests/unit/test_network_session.py::TestPipSession::test_proxy
Should Pass.
pytest -s tests/unit/test_network_session.py::TestPipSession::test_proxy --proxy valid_proxy
Should Fail.
pytest -s tests/unit/test_network_session.py::TestPipSession::test_proxy --proxy invalid_proxy

@uranusjr I know you ask me to add a news entry, but I'm not sure how/what exactly should I add. I know there is a NEWS.rst file you want me to add a new entry there, right? If so, could give me some guidance on this I would appreciate it.

[junqfisica]

Given the provided test will always fail on our CI, it's not much use IMO (anyone who can manually set a proxy to exercise the test can probably also set their proxy and just try pip out, after all...). My main intention in asking for a test was so that people without the ability to set up a proxy would be covered, because the test would run in CI to ensure this functionality still worked.

Question for the other @pypa/pip-committers - do we want to include this change without it being tested by our CI?

I'm ambivalent. The change is simple enough that asking for a test is making the whole PR turn into a much bigger exercise than it should be1. But on the other hand, if we do ever switch to another network library like httpx, how will we know that proxy support still works if we have no test?

Footnotes

1. And @junqfisica thanks for being patient in working with us on this. ↩

It is my pleasure @pfmoore to help and give a small contribution to such an important package to the python community as pip.

[pradyunsg]

If so, could give me some guidance on this I would appreciate it.

Happy to! Please see https://pip.pypa.io/dev/news-entry-failure.

This is mentioned in the CI failure for the news file check as well, although that hadn't been run when you asked your question. :)

[junqfisica]

@pradyunsg Thank you.

I've added a News entry 9691.bugfix.rst that makes reference to the issue #9691. Hope that's it.

[junqfisica]

@pradyunsg Could you check if this version passes all the workflows' checks? Thanks

[junqfisica]

Hi everyone,

First I would like to thank you all for the support on this PR.
Now that all checks have passed, is there any time estimative when this PR will be merged? :)