Migrate http proxy tests to pproxy #199

cdeler · 2020-09-28T18:07:10Z

In #196 comments it was mentioned that the first step of implementing socks proxy support might be replacing mitmproxy by pproxy (which allows us to test http, socks4 and socks5 support)

So that I rewrote the test fixtures and changed requirement file.

The next steps I propose to do after this PR:

rebase Implement socks5 proxy support using socksio #188 draft
move on acroding to the comment https://gitter.im/encode/community?at=5f6db8edf41f4105e4d7a9eb

florimondmanca

Looking good!

How come the HTTPS proxy test passes w/o further configuration or tweaks, though, whereas there are a few fixtures such as ca_ssl_context that relate to our previous HTTPS proxy setup? We ought to clean up any fixtures we don't use anymore, and see if there's anything we need to update in HTTPS proxies tests.

tests/conftest.py

florimondmanca · 2020-10-02T20:05:37Z

tests/conftest.py

-def example_org_cert_path(example_org_cert: trustme.LeafCert) -> typing.Iterator[str]:
-    with example_org_cert.private_key_and_cert_chain_pem.tempfile() as tmp:
-        yield tmp
+def proxy_server() -> typing.Iterator[URL]:


Instead of overriding the existing code, which makes things a bit hard to review (nothing blocking though), should we take the opportunity here and define any code related to the test proxy in conftest/utils.py? I think it's a good pattern to isolate anything that doesn't need to be defined in conftest.py to somewhere else, for readability. Bonus point here is the diff should be easier to review. :-)

I hope I understood you correctly. I've moved all the code apart from fixture initialization into the tests/utils.py

tests/conftest.py

florimondmanca · 2020-10-02T20:08:31Z

tests/conftest.py

-    The server is configured to use a trustme CA and key, this will allow our
-    test client to make HTTPS requests when using the ca_ssl_context fixture
-    above.
+        print(f"HTTP proxy started on http://{http_proxy_host}:{http_proxy_port}/")


Should we instead let the stdout/stderr of the pproxy process pass through?

Not sure which flag we'd need to pass to Popen(stdout=..., stderr=...), but that way we wouldn't need this print statement, and we'd be able to see the full output using controls built into pytest (such as the -s flag).

You are right, I was not consistent. Removed the print statement and the pipes

Removed unused fixtures. Co-authored-by: Florimond Manca <florimond.manca@gmail.com>

Co-authored-by: Florimond Manca <florimond.manca@gmail.com>

cdeler · 2020-10-03T19:59:17Z

@florimondmanca
I've removed trustme from requirements, but found that you are using it in the #205

florimondmanca · 2020-10-04T08:26:55Z

@cdeler Sounds good, feel free to drop anything you don't need as of this PR, I can add stuff back in the future. :)

florimondmanca

Great work! Two nits, but this is mergeable to me. Feel free to address whatever feels appropriate, and then we can get this merged. :)

florimondmanca · 2020-10-04T08:28:06Z

tests/conftest.py

-        yield (b"http", PROXY_HOST.encode(), PROXY_PORT, b"/")
-    finally:
-        thread.join()
+    with http_proxy_server(proxy_host, proxy_port) as proxy_server_fixture:


(Nit) How about as proxy_url, since it seems the context manager returns an URL tuple?

florimondmanca · 2020-10-04T08:31:48Z

tests/utils.py

@@ -11,6 +15,17 @@ def lookup_sync_backend():
    return "sync"


+def wait_until_pproxy_serve_on_port(host: str, port: int):


(Nit) Looks like there's nothing particular to pproxy here, so rather should we name this _wait_can_connect(host, port), or something more generic like that? (With _ indicating this is not a helper we're meaning to import from conftest or our tests.)

Good point, renamed

Co-authored-by: Florimond Manca <florimond.manca@gmail.com>

[1] encode/httpcore#199 git-svn-id: file:///srv/repos/svn-community/svn@722265 9fca08f4-af9d-4005-b8df-a31f2cc04f65

…changes (details below) * mitmproxy, trustme -> python-pproxy encode/httpcore#199 * pytest-asyncio, pytest-trio -> python-anyio encode/httpcore#181 Some tests still use pytest.mark.trio, though git-svn-id: file:///srv/repos/svn-community/svn@726093 9fca08f4-af9d-4005-b8df-a31f2cc04f65

cdeler mentioned this pull request Sep 28, 2020

Added pproxy test fixture, which allows to test requesting through socks proxy #196

Closed

cdeler force-pushed the migrate-http-proxy-tests-to-pproxy branch from a789c6b to 9c4edb3 Compare September 28, 2020 18:09

cdeler requested review from a team, florimondmanca and tomchristie September 28, 2020 19:41

cdeler force-pushed the migrate-http-proxy-tests-to-pproxy branch from 9c4edb3 to a6033ad Compare September 30, 2020 12:10

cdeler mentioned this pull request Oct 2, 2020

Implement socks5 proxy support using socksio #188

Closed

florimondmanca reviewed Oct 2, 2020

View reviewed changes

cdeler force-pushed the migrate-http-proxy-tests-to-pproxy branch 5 times, most recently from a265186 to 16efcc2 Compare October 3, 2020 19:15

cdeler and others added 3 commits October 3, 2020 22:15

Replaced mitmproxy by pproxy for proxy tests

6c258d6

Rebased onto master branch HEAD.

0e42d54

Removed unused fixtures. Co-authored-by: Florimond Manca <florimond.manca@gmail.com>

Apply suggestions from code review

f083e47

Co-authored-by: Florimond Manca <florimond.manca@gmail.com>

cdeler force-pushed the migrate-http-proxy-tests-to-pproxy branch from 16efcc2 to f083e47 Compare October 3, 2020 19:16

cdeler requested a review from florimondmanca October 3, 2020 19:22

florimondmanca approved these changes Oct 4, 2020

View reviewed changes

Apply suggestions from code review

95a4d3d

Co-authored-by: Florimond Manca <florimond.manca@gmail.com>

cdeler merged commit 73905ad into encode:master Oct 4, 2020

cdeler deleted the migrate-http-proxy-tests-to-pproxy branch October 4, 2020 09:11

cdeler mentioned this pull request Oct 4, 2020

Introduce Hypercorn as a local test server on 3.7+ #205

Merged

archlinux-github pushed a commit to archlinux/svntogit-community that referenced this pull request Oct 9, 2020

python-pproxy: checkdepends of upcoming python-httpcore

a8cbce3

[1] encode/httpcore#199 git-svn-id: file:///srv/repos/svn-community/svn@722265 9fca08f4-af9d-4005-b8df-a31f2cc04f65

archlinux-github pushed a commit to archlinux/svntogit-community that referenced this pull request Oct 9, 2020

python-pproxy: checkdepends of upcoming python-httpcore

62cdb64

[1] encode/httpcore#199 git-svn-id: file:///srv/repos/svn-community/svn@722265 9fca08f4-af9d-4005-b8df-a31f2cc04f65

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate http proxy tests to pproxy #199

Migrate http proxy tests to pproxy #199

cdeler commented Sep 28, 2020 •

edited

florimondmanca left a comment

florimondmanca Oct 2, 2020

cdeler Oct 3, 2020

florimondmanca Oct 2, 2020

cdeler Oct 3, 2020

cdeler commented Oct 3, 2020

florimondmanca commented Oct 4, 2020

florimondmanca left a comment

florimondmanca Oct 4, 2020

cdeler Oct 4, 2020

florimondmanca Oct 4, 2020

cdeler Oct 4, 2020

		@@ -11,6 +15,17 @@ def lookup_sync_backend():
		return "sync"


		def wait_until_pproxy_serve_on_port(host: str, port: int):

Migrate http proxy tests to pproxy #199

Migrate http proxy tests to pproxy #199

Conversation

cdeler commented Sep 28, 2020 • edited

florimondmanca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeler commented Oct 3, 2020

florimondmanca commented Oct 4, 2020

florimondmanca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeler commented Sep 28, 2020 •

edited