msg234488 - (view) |
Author: Guohua Ouyang (guohua) |
Date: 2015-01-22 11:50 |
Following the issue 7776, there is a patch for 2.7 version.
Which changes the method of class HTTPConnection from "_set_hostport" to "_get_hostport"[1], it seems introduce in some incompatibility issues.
On my system, the file "/usr/lib64/python2.7/site-packages/mercurial/url.py" from package mercurial-3.0-2.fc21 still use the old method "_set_hostport". I met an error "AttributeError: httpsconnection instance has no attribute '_set_hostport'" when use this package. I only see this incompatibility issue so far.
And in the file httplib.py itself, [2] still use "self._conn._set_hostport(host, port)", which should be "self._conn._get_hostport(host, port)" if it's settled to rename "_set_hostport" to "_get_hostport".
[1] https://github.com/python/cpython/blob/2.7/Lib/httplib.py#L743
[2] https://github.com/python/cpython/blob/2.7/Lib/httplib.py#L1132
|
msg234494 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2015-01-22 14:38 |
That leading underscore in the method name means it is not a public API and thus changes are allowed without any backwards-compatibility guarantees. Mercurial will need to update their code to handle this if they want to continue to use the method.
|
msg234499 - (view) |
Author: Mark Lawrence (BreamoreBoy) * |
Date: 2015-01-22 16:52 |
This is a bug first reported here https://mail.python.org/pipermail/python-list/2015-January/697228.html. The problem is that in #7776 r90728 568041fd8090 _set_hostport was renamed to _get_hostport but there is still a call to the former at line 1132 in httplib.py.
|
msg234504 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2015-01-22 17:45 |
There's definitely a bug because httplib is now using a method that doesn't exist.
|
msg234505 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2015-01-22 18:14 |
Ouch. Will fix this today. Strange, I think, no coverage exists for that and it has escaped our testing.
|
msg234508 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2015-01-22 18:41 |
@Senthil: If you're fixing this today, can you also correct the typo here: https://hg.python.org/cpython/rev/568041fd8090/#l1.27 (s/HTML/HTTP)? Just caught my eye going through the review that Mark linked.
|
msg234509 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2015-01-22 18:43 |
Demian, sure, will do.
|
msg234532 - (view) |
Author: Nikolaus Rath (nikratio) * |
Date: 2015-01-23 01:21 |
Serhiy, did you add me to Cc just for information, or is there anything I should be doing (having written the patch that introduced this bug)?
|
msg234536 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2015-01-23 01:47 |
Nikolaus, nothing might be required from your end. Just a good protocol to keep the interested parties in CC.
|
msg234546 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2015-01-23 09:13 |
Here is the patch to fix this. I have added a test case covering this change. Please review this and if it is good to go, I will commit it. Thank you.
|
msg234549 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2015-01-23 09:36 |
+class TestServer(TestCase):
This can be a mixin.
+ def testHTTPWithConnectHostPost(self):
Post -> Port?
+ self.conn = httplib.HTTP(host=constructor_host, port=constructor_port)
or to make the test simpler,
self.conn = httplib.HTTP()
|
msg234559 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2015-01-23 15:15 |
Other than Berker's comments, LGTM.
|
msg234568 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2015-01-23 18:08 |
Addressed Berker's review comments.
1) Made the TestServer a Mixin. (Thanks, that's the correct to do).
2) Changed Post to Port.
3) I went with still using a testdomain and port in the constructor. My idea of the test is to demonstrate that the connect(host,port) is used when given rather than HTTP(host,port).
|
msg234569 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2015-01-23 18:31 |
LGTM.
|
msg234636 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2015-01-24 20:59 |
New changeset c2bba3c9424b by Senthil Kumaran in branch '2.7':
Fix Issue23300 : httplib.HTTP classe's connect method should use _get_hostport
https://hg.python.org/cpython/rev/c2bba3c9424b
|
msg234637 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2015-01-24 21:00 |
Fixed in 2.7. Other versions do not have this bug.
|
msg234638 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2015-01-24 21:01 |
On Sat, Jan 24, 2015, at 16:00, Senthil Kumaran wrote:
>
> Senthil Kumaran added the comment:
>
> Fixed in 2.7. Other versions do not have this bug.
Thank you! Would porting the tests be useful, though?
|
msg234639 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2015-01-24 21:05 |
> Would porting the tests be useful, though?
Yes, it will be useful to port the tests to 3.4 and default branch.
I will do that.
|
msg234642 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2015-01-25 00:43 |
Actually, the same tests cannot be ported to 3.4 or default. httplib.HTTP class has been merged to HTTPConnection in 3.x and we don't allow a host, and port in connect().
There is good test coverage for verifying host, port in test_hort_port test in 3.x
However, I saw that TunnelTests could see some refactor and a test addition to cover the _tunnel_host, _tunnel_port attributes (which are set using _get_hostport method).
Attaching a change to client.test_httplib which refactors TunnelTest and adds a new test called test_set_tunnel_host_port_headers. Please review if this change is Okay. I can push this in 3.4 and default branches.
Thanks!
|
msg234645 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2015-01-25 01:34 |
+ self.assertDictEqual(self.conn._tunnel_headers, tunnel_headers)
You could just use assertEqual. Quoting from docs:
"The list of type-specific methods automatically used by assertEqual() are summarized in the following table. Note that it’s usually not necessary to invoke these methods directly."
https://docs.python.org/3/library/unittest.html#unittest.TestCase.addTypeEqualityFunc
+ self.assertTrue(b'CONNECT destination.com' in self.conn.sock.data)
+ self.assertTrue(b'Host: destination.com' in self.conn.sock.data)
You could use assertIn instead.
|
msg234646 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2015-01-25 01:38 |
Thanks for the review comments. Updated the patch with the suggested changes.
|
msg234649 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2015-01-25 02:54 |
LGTM.
|
msg234650 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2015-01-25 03:27 |
Committed the test changes in fcab9c106f2f (3.4) and a858cde113f2 (3.5)
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:12 | admin | set | github: 67489 |
2015-01-25 03:27:37 | orsenthil | set | status: open -> closed
messages:
+ msg234650 |
2015-01-25 02:54:17 | berker.peksag | set | messages:
+ msg234649 |
2015-01-25 01:38:03 | orsenthil | set | files:
+ 23300-3.3-v2.patch
messages:
+ msg234646 |
2015-01-25 01:34:22 | berker.peksag | set | messages:
+ msg234645 |
2015-01-25 00:43:07 | orsenthil | set | files:
+ 23300-3.3.patch
messages:
+ msg234642 |
2015-01-24 21:05:01 | orsenthil | set | status: closed -> open
messages:
+ msg234639 |
2015-01-24 21:01:47 | benjamin.peterson | set | messages:
+ msg234638 |
2015-01-24 21:00:31 | orsenthil | set | status: open -> closed resolution: fixed messages:
+ msg234637
stage: commit review -> resolved |
2015-01-24 20:59:05 | python-dev | set | nosy:
+ python-dev messages:
+ msg234636
|
2015-01-23 18:31:06 | berker.peksag | set | messages:
+ msg234569 stage: patch review -> commit review |
2015-01-23 18:08:57 | orsenthil | set | files:
+ 23000-v2.patch
messages:
+ msg234568 |
2015-01-23 15:15:14 | demian.brecht | set | messages:
+ msg234559 |
2015-01-23 09:36:07 | berker.peksag | set | nosy:
+ berker.peksag messages:
+ msg234549
|
2015-01-23 09:13:16 | orsenthil | set | files:
+ 23000.patch keywords:
+ patch messages:
+ msg234546
stage: needs patch -> patch review |
2015-01-23 01:47:45 | orsenthil | set | messages:
+ msg234536 |
2015-01-23 01:21:54 | nikratio | set | messages:
+ msg234532 |
2015-01-22 18:43:42 | orsenthil | set | messages:
+ msg234509 |
2015-01-22 18:41:44 | demian.brecht | set | nosy:
+ demian.brecht messages:
+ msg234508
|
2015-01-22 18:14:36 | orsenthil | set | assignee: orsenthil messages:
+ msg234505 |
2015-01-22 18:09:25 | serhiy.storchaka | set | nosy:
+ nikratio, serhiy.storchaka
type: enhancement -> behavior stage: needs patch title: An improper change in httplib.py -> httplib is using a method that doesn't exist |
2015-01-22 17:45:42 | benjamin.peterson | set | status: closed -> open
nosy:
+ benjamin.peterson, orsenthil messages:
+ msg234504
resolution: wont fix -> (no value) |
2015-01-22 16:52:56 | BreamoreBoy | set | nosy:
+ BreamoreBoy messages:
+ msg234499
|
2015-01-22 14:38:23 | brett.cannon | set | status: open -> closed
nosy:
+ brett.cannon messages:
+ msg234494
resolution: wont fix |
2015-01-22 11:50:08 | guohua | create | |