-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
Remove py2 tests #4114
Remove py2 tests #4114
Conversation
@@ -1,63 +1,78 @@ | |||
import json | |||
import os | |||
import re | |||
from subprocess import Popen, PIPE | |||
import sys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💄 I think the subprocess
import should be below sys
, being in the from … import …
form. See https://stackoverflow.com/a/20763446/939364
Codecov Report
@@ Coverage Diff @@
## master #4114 +/- ##
==========================================
+ Coverage 83.69% 83.72% +0.03%
==========================================
Files 165 165
Lines 9717 9717
Branches 1445 1445
==========================================
+ Hits 8133 8136 +3
+ Misses 1336 1333 -3
Partials 248 248
|
tests/test_proxy_connect.py
Outdated
@@ -67,6 +81,7 @@ def test_https_connect_tunnel(self): | |||
yield crawler.crawl(self.mockserver.url("/status?n=200", is_secure=True)) | |||
self._assert_got_response_code(200, l) | |||
|
|||
@pytest.mark.xfail(reason='mitmproxy gives an error for noconnect requests') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What error is that? Is it some feature missing in mitmproxy, which was present before? If so, it'd be good to open a ticket in their issue tracker, and add a link here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Invalid request scheme: https" like in mitmproxy/mitmproxy#848.
See #2545 (comment)
Note that the mitmproxy issue above is open but I'm not 100% sure it's the same use case. Actually, I'm not sure what is the use case for the Scrapy code to skip CONNECT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kmike I've added a comment about this as requested
2ea7544
to
44d6677
Compare
44d6677
to
beb7d80
Compare
Thanks @wRAR! I'm merging it; could you please open an issue to add noconnect tests back, to keep track of it (even if we decided not to add them back)? |
This removes Python2-only tests and updates test_proxy_connect.py to new mitmproxy.
Fixes #2545.