Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable test_proxy_connect in Python 3 #2545

Closed
kmike opened this issue Feb 8, 2017 · 14 comments · Fixed by #4114
Closed

enable test_proxy_connect in Python 3 #2545

kmike opened this issue Feb 8, 2017 · 14 comments · Fixed by #4114

Comments

@kmike
Copy link
Member

@kmike kmike commented Feb 8, 2017

mitmproxy is ported to Python 3 (in fact, it is now Python3-only), so it'd be nice to enable test_proxy_connect.py in Python 3.

@ShivramIITG
Copy link

@ShivramIITG ShivramIITG commented Feb 10, 2017

Hey, i am new to open source, this is the first issue i would work on (hopefully). Is this a beginner level issue? If so, i would like to start working on it. Kindly guide me.

@kai3n
Copy link

@kai3n kai3n commented Feb 11, 2017

Me too, I am new and want to contribute this open source.. Could you recommend some beginner level issue?

@kmike
Copy link
Member Author

@kmike kmike commented Feb 14, 2017

Hey @ShivramIITG and @kai3n! Yes, I think this is a good isses for a beginner. It is not a super-easy issue, but it is clear what the end result should be.

I think to work on it one have to

  1. figure out how to run Scrapy tests (the whole testing suite and individual tests);
  2. check that test_proxy_connect.py tests pass in Python 2;
  3. enable them in Python 3 (remove a line from py3-ignores.txt) and run;
  4. figure out why are they failing (likely because mitmproxy is not installed or is not available in Python 3);
  5. add new mitmproxy (or some of its related libraries, I'm not sure - they refactored the code structure quite a lot) to tox.ini Python 3 environments, keeping old mitmproxy for Python 2 environments;
  6. figure out what changed in mitmproxy and how to port tests to the new version, so that they work both in Python 2 (with an old version) and Python 3 (with the new version).

Maybe there is a way to use only a new mitmproxy version, I haven't checked that - it'd be ideal.

@Parth-Vader
Copy link
Member

@Parth-Vader Parth-Vader commented Feb 18, 2017

@kmike I ran the whole test for both python2 and python3.
For python2, only test_proxy_connect.py ran (among the proxy ones) while no proxy tests for python3.

But I, currently, am using a proxy network.Does it affect the test?

@Parth-Vader
Copy link
Member

@Parth-Vader Parth-Vader commented Feb 18, 2017

Doing sudo tox -e py35 -- tests/test_proxy_connect.py
gave the error :
ImportError: No module named 'libmproxy'

And seeing this Move to Python 3, I believe it hasn't been moved to python 3.

EDIT: Doing the above for python2 gave py27: commands succeeded congratulations :).
So,for python3, we need to either change libmproxy settings or find an alternative, right?

@Parth-Vader
Copy link
Member

@Parth-Vader Parth-Vader commented Feb 21, 2017

ping @kmike
What has to be done next?

@kmike
Copy link
Member Author

@kmike kmike commented Feb 21, 2017

Hey @Parth-Vader,

The ticket you linked to says that mitmproxy is moved to Python 3 (but Python 2 support is dropped). Sowe need a new version of mitmproxy in Python 3 environments and old version of mitmproxy in Python 2 environments,

@Parth-Vader
Copy link
Member

@Parth-Vader Parth-Vader commented Feb 21, 2017

@kmike Alright, I will take it.
What is the workflow? Should I make a new branch and then send a PR?

@kmike
Copy link
Member Author

@kmike kmike commented Feb 21, 2017

@Parth-Vader thanks! Yes, creating a new branch and sending a PR is the way to go.

@Parth-Vader
Copy link
Member

@Parth-Vader Parth-Vader commented Feb 23, 2017

Ping @kmike

Progress till now :-

  • Python 3 support from mitmproxy starts from version 0.18.1.

  • The library libmproxy has changed to mitproxy from versions 0.16 and so have libmproxy.controller and libmproxy.proxy.

  • I am successful in running the test_proxy_connect.py for python3, although all the 6 tests fail for it.

The tests fail since libmproxy has been changed and controller and proxy have to be replaced.

Now, I'll try to make the tests pass.

@wRAR
Copy link
Contributor

@wRAR wRAR commented Aug 29, 2019

Related: mitmproxy/mitmproxy#2061

@wRAR
Copy link
Contributor

@wRAR wRAR commented Sep 2, 2019

I rewrote the test to use the modern mitmproxy in a thread but couldn't make it process the connections, I think it's because the asyncio event loop it uses should be running in the main thread, not a separate one.

Then I replaced that with running mitmdump in a separate process and some tests now pass while some fail.

Both noconnect tests fail with "Invalid request scheme: https" and the localhost:99999 test fails early, with "ValueError: Port out of range 0-65535".

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Sep 17, 2019

@wRAR Should we just update expectations then?

@wRAR
Copy link
Contributor

@wRAR wRAR commented Sep 17, 2019

@Gallaecio so there are two cases (in three tests) here.

First, in test_https_connect_tunnel_error: we pass an incorrect URL (with a port 99999) to the crawler and expect a scrapy.core.downloader.handlers.http11.TunnelError because the proxy returns 400 Bad Request. On Python 3 this URL is not even passed to the proxy. We can find a different URL with the required properties and use it instead.

Second, in test_https_noconnect and test_https_noconnect_auth_error: we use a proxy URL with ?noconnect appended, so that Scrapy uses GET with the proxy instead of CONNECT, just like it does with HTTP. Current mitmproxy answers these requests with "Invalid request scheme: https" and I don't know if it supports this mode of operation at all, it's hard to find info on this mode, mitmproxy-related or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants