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

Remove py2 tests #4114

Merged
merged 12 commits into from Nov 21, 2019
Merged

Remove py2 tests #4114

merged 12 commits into from Nov 21, 2019

Conversation

wRAR
Copy link
Member

@wRAR wRAR commented Oct 31, 2019

This removes Python2-only tests and updates test_proxy_connect.py to new mitmproxy.

Fixes #2545.

@wRAR wRAR added the Python 3 label Oct 31, 2019
@@ -1,63 +1,78 @@
import json
import os
import re
from subprocess import Popen, PIPE
import sys
Copy link
Member

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

@wRAR wRAR mentioned this pull request Oct 31, 2019
@wRAR wRAR added this to the v2.0 milestone Oct 31, 2019
@codecov
Copy link

codecov bot commented Oct 31, 2019

Codecov Report

Merging #4114 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            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
Impacted Files Coverage Δ
scrapy/core/downloader/handlers/http11.py 92.91% <0%> (+1.18%) ⬆️

tests/test_proxy_connect.py Outdated Show resolved Hide resolved
tests/test_proxy_connect.py Outdated Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
@@ -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')
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

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

@wRAR wRAR force-pushed the remove-py2-tests branch 2 times, most recently from 2ea7544 to 44d6677 Compare November 18, 2019 16:30
@kmike
Copy link
Member

kmike commented Nov 21, 2019

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)?

@kmike kmike merged commit d2b73b8 into master Nov 21, 2019
@wRAR wRAR deleted the remove-py2-tests branch November 18, 2022 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable test_proxy_connect in Python 3
4 participants